Runtime: API proposal: Allow JavaScriptEncoder to have relaxed escaping rules

[GrabYourPitchforks]

There are a few outstanding work items (mostly being tracked by https://github.com/dotnet/corefx/issues/37192 and issues linked from there) where for particular scenarios it would be ideal for the JSON serializer to relax the default escaping restrictions a bit.

API Proposal

namespace System.Text.Encoding.Web
{
    // EXISTING type
    public class JavaScriptEncoder
    {
        // NEW overload of EXISTING method
        public static JavaScriptEncoder Create(TextEncoderSettings settings, JavaScriptEscapingBehavior escapingBehavior);
    }

    // NEW type
    public enum JavaScriptEscapingBehavior
    {
        Default, // same logic as used by JavaScriptEncoder.Default
        RelaxedJsonEscaping // relaxed behaviors as described below
    }
}

Existing behavior

The JavaScriptEncoder type currently has the following logic to determine what Unicode scalar values to escape.

1. Take the bitmap provided via Create(TextEncoderSettings) , or create a new bitmap from Create(UnicodeRange[]). This bitmap lists all code points which are _candidates_ for the allow list. (This list will be further filtered.) If using JavaScriptEncoder.Default, it's the same as having called Create(UnicodeRange.BasicLatin).

2. Forbid all Unicode characters which aren't in the "allowed categories" list, including control characters, non-ASCII whitespace, and so on. The full list of allowed categories can be found at https://github.com/dotnet/corefx/blob/bd30a3f458b6b0f71204fc3630b2d29b780c4167/src/System.Text.Encodings.Web/tools/GenDefinedCharList/Program.cs#L154. By "forbid", I mean that if any of these characters exist in the bitmap provided by step (1) they're removed from the bitmap.

3. Forbid all HTML-sensitive characters: <, >, &, etc. Forbid UTF-7 sensitive characters like +.

4. Forbid all JavaScript string-sensitive characters and JSON-sensitive characters: ", \, etc. Single-quote and backtick are also captured by this for compatibility with ECMAScript.

5. Additionally, when escaping certain characters like ", the library emits \u0022 instead of \" to provide extra protection when contained within an HTML wrapper or other envelope where the " character is itself an escape character.

New behavior

If the new enum value RelaxedJsonEscaping is provided, the behavior of each of the above lines changes as described below.

1. HTML-sensitive characters like <, >, &, etc. aren't forbidden unless they're also JSON-sensitive. The + character isn't forbidden. Whether these characters are escaped is controlled entirely by the bitmap provided by step (1).

2. Single-quote and backtick aren't forbidden. If they're allowed by the bitmap provided by step (1), they can pass through unescaped.

3. When escaping " and ' (if not allowed), the library emits \" and \' instead of \uXXXX. No extra envelope protection is afforded.

4. If the default escaper forbids characters like ″ (U+2033) in a future release to protect against best-fit mapping attacks, the relaxed escaper may instead opt to allow these characters to come through unescaped.

Discussion

Consumers will be able to opt in to this behavior. ASP.NET and the SDK (see https://github.com/dotnet/core-setup/issues/7137) will probably want to do so in certain cases. I recommend we maintain the aggressive behavior of JavaScriptEncoder.Default because it does provide defense-in-depth against vulnerabilities which are common in real-world applications. See my comment at https://github.com/dotnet/corefx/issues/38354#issuecomment-504780719 for more information.

If possible, I recommend that we give this enum value a name which conveys the full weight of "Hey, you're disabling default protections. Please consider your entire scenario top-to-bottom and only proceed if you know for certain the protections afforded aren't applicable to your scenario." I believe that the majority of our developer audience won't understand the full consequences of flipping this switch, and I don't want to end up in a world where well-meaning developers inadvertently introduce vulnerabilities into their web applications.

Internally, we can modify SDL to have a rule that goes something along the lines of "The JavaScriptEncoder type is SDL-approved unless you have passed the RelaxedJsonEscaping enum value. All usages of this enum value must be reviewed to avoid introducing vulnerabilities."

[tarekgh]

@GrabYourPitchforks I assume this is needed for System.Text.Json. Usually we tag any issue with only one area tag for reporting purpose. if you are ok, which area tag we should remove? I would say keep only area-System.Text.Encodings.Web.

[GrabYourPitchforks]

@tarekgh If we need to keep only one label, keep _System.Text.Encodings.Web_.

[steveharter]

Capturing this thought: we should consider adding a fast mode for S.T.Json that can detect this mode and\or determine what the ascii encodings are and use those in a fast mode.

Background:
There is a fast implementation of ascii encoding in S.T.Json that executes in 70% of the time than S.T.Encodings.Web ascii after this PR is in -- so if it takes 100ms in S.T.Json to encode ascii, S.T.Encodings.Web will take ~140ms. This includes optimizing both implementations done by that PR, and is only for measuring ascii characters < 128.

The fast code is executed when there is no default escaper specified (on the serializer options or writer options). When a character >= 128 is encountered we switch to the slower S.T.Encodings.Web default escaper to handle the escaping. We ensure the escaping is the same for ascii between the two implementations.

So if we add a JavaScriptEscapingBehavior knob like this and users specifies that encoder on the serializer options or writer options, they will have slower escaping even if escaping fewer characters because the fast code in S.T.Json will not run. Thus the desire to add a fast mode to S.T.Json to detect this knob.

[nguerrera]

I can't tell how '/' will be handled from above. Will it be allowed?

[GrabYourPitchforks]

we should consider adding a fast mode for S.T.Json that can detect this mode and\or determine what the ascii encodings are and use those in a fast mode.

I agree with this. I have a few gists that were attempting to solve this problem, but I have no cycles to get to this in the 3.0 timeframe.

I can't tell how '/' will be handled from above. Will it be allowed?

@nguerrera We've been discussing adding / to the default allow list so it will go through as-is unescaped. I opened https://github.com/dotnet/corefx/issues/39445 to clarify matters.

[GSPP]

It is very valuable to be able to obtain the smallest possible encoding. Sometimes, JSON is stored to a database where space usage is a concern. This feature is great.

[GrabYourPitchforks]

@GSPP Saying that you want to persist raw JSON to a database makes me want to fast-track the work to escape best-fit mapping characters like ″ (U+2033). :)

[krwq]

@GrabYourPitchforks is there also a plan to let user customize it (i.e. make some stuff virtual to allow override; not necessarily right now but we should think about what will happen in the future when designing API)?

[GrabYourPitchforks]

@krwq There are already virtual methods on the TextEncoder and JavaScriptEncoder types that allow full customization. That's how our unit tests work today.

The API as introduced in 1.0 was not really intended for people to subclass easily given the API surface as it exists today. I wanted to introduce the break in the 3.0 timeframe (see https://github.com/dotnet/corefx/issues/33509) to make it easier for folks to customize now that JSON is also taking a dependency on it, but it keeps getting punted to future milestones. We can consider it again for 5.0, but since it was already causing a lot of grief in the 3.0 timeframe I'm not sure if folks will be able to stomach the work in the 5.0 timeframe.

[JamesNK]

My point of view: escaping every unicode character isn't a good user experience. For non-English developers it is a very bad experience (JSON is much bigger, completely unreadable).

It sounds like something people can fix in 3.0 with their own encoder, but the library shouldn't require people do work to get a sensible default experience. It should come out of the box.

As a scenario: "Hello world" in Japanese/Chinese/Russian in JSON shouldn't be escaped by default.

[GrabYourPitchforks]

As a scenario: "Hello world" in Japanese/Chinese/Russian in JSON shouldn't be escaped by default.

It shouldn't be. If you're using ASP.NET and you inspect the response, these characters shouldn't be escaped. Additionally, per our earlier conversation with the SDL team, if you're using UTF-16 encodings (e.g., you call JsonSerializer.ToString(some_input)), it's also fine to keep CJK characters unescaped. (This isn't the behavior today, but I'm working with the SDL team to get this marked as acceptable. It'd also allow addressing https://github.com/dotnet/corefx/issues/38354 to a large extent.)

The scenario which __isn't__ an acceptable default per our security guidelines is having an API which returns or emits raw bytes, where those bytes represent unescaped non-ASCII data.

[terrajobst]

After some discussion, we see a few issues:

• The enum approach doesn't seem to be scalable. It would need to be a flags-based enum, otherwise it's hard to add more orthogonal options later.
• If we don't care for this to be extensible, the easiest option is to offer an overload or a new static property that returns an instance that escapes fewer characters. Given that we don't need to tweak anything else, the property seems to be right approach.
• The point of the encoders is to protect consumers for known encoding attacks; thus, escaping fewer things has security implications. Yes, it might make your JSON payloads prettier, but it comes with a price tag. We believe this can be addressed by naming (not by documentation alone, though).

C# namespace System.Text.Encoding.Web { public partial class JavaScriptEncoder { public static JavaScriptEncoder UnsafeRelaxedJsonEscaping { get; } } }

[karelz]

Fixed in master (3.0) in PR dotnet/corefx#39550 before final integration into release/3.0 branch.