ESAPI / esapi-java-legacy

ESAPI (The OWASP Enterprise Security API) is a free, open source, web application security control library that makes it easier for programmers to write lower-risk applications.
https://owasp.org/www-project-enterprise-security-api/
Other
610 stars 368 forks source link

JSON string escaping support #722

Closed noloader closed 2 years ago

noloader commented 2 years ago

This commit adds JSON string escaping support per RFC 8259, Section 7.

The encoder escapes special characters using two-character escape sequences for popular characters (like \r, \n and \t). For other characters which require escaping, the six-character representation is used (like \u0000).

noloader commented 2 years ago

Hi Everyone/@kwwall,

We need this encoder at $dayjob.

kwwall commented 2 years ago

I will take a look, but no promises that I can get this into the 2.5.0.0 release, which I am trying to get out today.

Also, most devs that I've seen just use the GSON library for this use case because they are already using it for most other JSON related use cases.

-kevin

On Sun, Jul 17, 2022, 11:55 AM Jeffrey Walton @.***> wrote:

Hi @.*** https://github.com/kwwall,

We need this encoder at $dayjob.

— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/pull/722#issuecomment-1186551805, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PG6B7OX2HRYXZ4TKLN3VUQUHPANCNFSM53Z6OG3Q . You are receiving this because you were mentioned.Message ID: @.***>

noloader commented 2 years ago

@kwwall,

I will take a look, but no promises that I can get this into the 2.5.0.0 release, which I am trying to get out today.

No problems. The release is more important.

The encoder is available if you want it. If not, we can carry patches for it now that it is available.

xeno6696 commented 2 years ago

I will take a look, but no promises that I can get this into the 2.5.0.0 release, which I am trying to get out today. Also, most devs that I've seen just use the GSON library for this use case because they are already using it for most other JSON related use cases. -kevin

@kwwall It's not a make/break feature as in the past I've just had people do javascript escapes for JSON payload data. It's overkill but it works. This one is lighter weight.

noloader commented 2 years ago

@xeno6696,

I'd like to see this altered to use the AbstractIntegerCodec ...

Done. I rebased and pushed an updated version.

I'm still not convinced this codec handles all cases well. I'm going to get some test cases added for U+1F602 and friends.

noloader commented 2 years ago

Hi Everyone,

One question I have.... I used IllegalArgumentException for an encoding error when decoding. Should this use an ESAPI-specific exception?

I tried to use EncodingException, but I encountered an error. It think it is the same error from GH #227.

xeno6696 commented 2 years ago

@noloader I'm a bit scared of the --force push commits. Typically with git it's a major red flag.

What was necessitating the use of the command?

noloader commented 2 years ago

Hi @xeno6696,

I'm a bit scared of the --force push commits. Typically with git it's a major red flag

What was necessitating the use of the command?

Rebasing to flatten out history to one commit.

I don't think it effects ESAPI's history. It only effects my fork. From ESAPI's view of the world, there's one commit in the PR against ESAPI's d6251b5e5ade0bfe312efbb1d798af266ec70e0d. d6251b5e is tip of develop, which is what Kevin pushed yesterday.

kwwall commented 2 years ago

@xeno6696 wrote:

@kwwall It's not a make/break feature as in the past I've just had people do javascript escapes for JSON payload data. It's overkill but it works. This one is lighter weight.

Plus somewhat ironically, not EVERY use of JSON is associated with JavaScript any more. (At least in the usual sense of JavaScript use.) JSON is now one of the many ubiquitous information preserving file formats used much like CSV and XML files are to move information around, so I think people would favor something lighter weight.

kwwall commented 2 years ago

@noloader wrote:

One question I have.... I used IllegalArgumentException for an encoding error when decoding. Should this use an ESAPI-specific exception?

I tried to use EncodingException, but I encountered an error. It think it is the same error from GH #227.

I don't think we should tightly couple our various codecs to ESAPI exceptions, especially since some of those are tied to ESAPI's Intrusion Detector which are tied to ESAPI Logging. I never really liked that. Using ESAPI exceptions and other ESAPI components is fine under org.owasp.esapi.reference but I prefer loose-coupling elsewhere. I try even avoid ESAPI interfaces for those spots.

Let's just keep it simple. I think that IllegalArgumentException works fine.

noloader commented 2 years ago

Let's just keep it simple. I think that IllegalArgumentException works fine.

Perfect, thanks.

planetlevel commented 2 years ago

Perhaps an Observable type model would be better for intrusion relevant events.

—Jeff

Jeff Williams 410-707-1487


From: Jeffrey Walton @.> Sent: Monday, July 18, 2022 5:00:57 PM To: ESAPI/esapi-java-legacy @.> Cc: Subscribed @.***> Subject: Re: [ESAPI/esapi-java-legacy] JSON string escaping support (PR #722)

Let's just keep it simple. I think that IllegalArgumentException works fine.

Perfect, thanks.

— Reply to this email directly, view it on GitHubhttps://github.com/ESAPI/esapi-java-legacy/pull/722#issuecomment-1188305461, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAUUFTGJZ5DIXAGXU5IZ55TVUXAYTANCNFSM53Z6OG3Q. You are receiving this because you are subscribed to this thread.Message ID: @.***>

noloader commented 2 years ago

not EVERY use of JSON is associated with JavaScript any more...

++. Javascript encoding is not valid JSON encoding. A conforming JSON parser should reject \x2f and friends.

kwwall commented 2 years ago

Perhaps an Observable type model would be better for intrusion relevant events.

Hindsight is 20/20, but something to consider in ESAPI 3.

kwwall commented 2 years ago

@noloader - Just wondering, since you are doing this codec and JSON has become the linga franca of transporting data, for completeness, you might want to consider writing some new Validator methods, like isValidSafeJSON() that would be analogous to isValidSafeHTML().

xeno6696 commented 2 years ago

@noloader - Just wondering, since you are doing this codec and JSON has become the linga franca of transporting data, for completeness, you might want to consider writing some new Validator methods, like isValidSafeJSON() that would be analogous to isValidSafeHTML().

AntiSamy does JSON now?

kwwall commented 2 years ago

No, AFAIK, AntiSamy does NOT do JSON, which is why I asked if @noloader wanted to do it.

On Wed, Jul 27, 2022, 12:12 AM Matt Seil @.***> wrote:

@noloader https://github.com/noloader - Just wondering, since you are doing this codec and JSON has become the linga franca of transporting data, for completeness, you might want to consider writing some new Validator methods, like isValidSafeJSON() that would be analogous to isValidSafeHTML().

AntiSamy does JSON now?

— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/pull/722#issuecomment-1196245440, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PG6C6TV6G43BP4INQJDVWCZMHANCNFSM53Z6OG3Q . You are receiving this because you were mentioned.Message ID: @.***>

xeno6696 commented 2 years ago

@kwwall AntiSamy only handles HTML, that ask amounts to rewriting/cloning AntiSamy centering it around JSON parsing.

And THEN you have the mixed context: AntiSamy is designed to allow websites to safely handle user-inputted HTML. Our use-case derives from that. In what context does a similar use case exist for JSON? What scripts can you execute using JSON’s non-existent script tag? What are you defending against, precisely? What would be the json equivalent to the tag whitelisting AntiSamy does?

JSON’s better viewed as a transport layer data format, not a browser renderable scripting language like HTML or JavaScript. JSON could handle data intended to be used in SQL or more likely, javascript itself or HTML intended to be rendered by innerHTML in javascript.

The safeValidHTML contract works because HTML is the single explicit target. With JSON you have everything. You’re asking for an XSS servlet filter. To drill that point home, why don’t we have a safeValidXML call which would be the closest equivalent to JSON?

planetlevel commented 2 years ago

I generally agree with Matt here – since JSON and XML are just data they can’t contain attacks so AntiSamy wouldn’t be applicable.

Except that it’s not quite true. XML can have doctypes that contain XXE attacks – would certainly be a nice thing to strip out. And JSON is actually javascript and some people might eval it. So you could imagine sanitizing it so that only data is left.

I’m not recommending that we build it, because it’s hard. You probably couldn’t use an existing parser, because it might execute the code in the data. So you’d have to build a custom one. Which is quite difficult to get right. I’d steer folks away from trying to sanitize XML and JSON and simply ensuring that those data formats are used safely.

--Jeff

From: Matt Seil @.> Date: Wednesday, July 27, 2022 at 9:21 AM To: ESAPI/esapi-java-legacy @.> Cc: Jeff Williams @.>, Comment @.> Subject: Re: [ESAPI/esapi-java-legacy] JSON string escaping support (PR #722)

@kwwallhttps://github.com/kwwall AntiSamy only handles HTML, that ask amounts to rewriting/cloning AntiSamy centering it around JSON parsing.

And THEN you have the mixed context: AntiSamy is designed to allow websites to safely handle user-inputted HTML. Our use-case derives from that. In what context does a similar use case exist for JSON? What scripts can you execute using JSON’s non-existent script tag? What are you defending against, precisely? What would be the json equivalent to the tag whitelisting AntiSamy does?

It’s better viewed as a transport layer data format, not a browser renderable scripting language like HTML or JavaScript. JSON could handle data intended to be used in SQL or more likely, javascript itself or HTML intended to be rendered by innerHTML in javascript.

The safeValidHTML contract works because HTML is the single explicit target. With JSON you have everything. You’re asking for an XSS servlet filter. To drill that point home, why don’t we have a safeValidXML call which would be the closest equivalent to JSON?

— Reply to this email directly, view it on GitHubhttps://github.com/ESAPI/esapi-java-legacy/pull/722#issuecomment-1196756098, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAUUFTAUHOPW4XOPJV5UCXLVWEZWLANCNFSM53Z6OG3Q. You are receiving this because you commented.Message ID: @.***>

kwwall commented 2 years ago

@xeno6696 and @planetlevel - Yeah, I went overboard with the "valid safe JSON". What I was really thinking is maybe just a check to see if a string is valid JSON, which ought to be a lot simpler. A Validator.isValidJSON() if you will, just for completeness.

I know you can do that with Gson like this:

import com.google.gson.Gson;

public final class JSONUtils {
  private static final Gson gson = new Gson();

  private JSONUtils(){}

  public static boolean isJSONValid(String jsonInString) {
      try {
          gson.fromJson(jsonInString, Object.class);
          return true;
      } catch(com.google.gson.JsonSyntaxException ex) { 
          return false;
      }
  }
}

so, using other 3rd party FOSS libraries, it's not really difficult.

The question is, how difficult is it if we were to write our own parser? (Because I don''t really want to pull in Gson as a dependency just for this purpose, even though Gson is well-maintained and way secure than alternative like, say, FasterXML Jackson Databind.) But if it didn't require a lot of code (I think we would just parse it, but not bother to deserialize it into objects), it may be worth considering.

Anyhow, sorry for the confusion.

xeno6696 commented 2 years ago

@kwwall the safest way to accomplish that would be to use javacc to build a JSON parser of our own. Nobody rolls their own parsers and lexers these days. This is incomplete but is where I usually begin when I sit down to do one of these. (It's been 8yrs since the last time I did it.)

@planetlevel has disliked the idea in the past because browsers for example are notorious for breaking BNF rules by trying to "help" by doing things like fixing invalid HTML. That's valid. But I think the risk would be less in the case of JSON because of its simplicity and lack of things like XXE attacks. Also, we should call out that the best we can do is ensure that the JSON is legally parseable--which is what you're suggesting with your GSON example.