12 ms·

The unexpected Google wide domain check bypass

644 points by notRobot 7y ago

zeta0134 7y ago
Something something regex and two problems. \s Seriously though, nice find! It's often surprising how tiny little curiosities can lead down the rabbit hole to such interesting problems. In this case, it was a fairly logical flawed assumption, but the mess of regex made it just hidden enough to be tough to spot.
- jcims 7y ago
  This is one area where bug bounty programs can really outperform an internal security function. There are others, generally, but this deep rabbit hole exploration is one of the best.
- hinkley 7y ago
  > \s Too soon.
kyle-rb 7y ago
I love that the fix is simply a quadruple-backslash (aka an "actual backslash, for real this time" per https://xkcd.com/1638/ https://xkcd.com/1638/)
- gulbrandr 7y ago
  Thanks for the xkcd link, I loved it!
- hinkley 7y ago
  More than a couple of times have I chosen a different solution to a problem (especially shell scripts) to avoid having to double backslashes more than once. Because fuck trying to explain how that code actually works to anybody else, or myself a year from now.
chatmasta 7y ago
This was a really enjoyable, thorough and readable write-up. Thanks, and nice work!
- tanseydavid 7y ago
  This is the exact comment I would have written if @chatmasta had not already posted it. Thanks for the write-up.
- hnick 7y ago
  I'll add that I usually have trouble following these, but it was a breeze here. Glad I read it.
yvoschaap 7y ago
Great find! True hacker news worthy.
ipsum2 7y ago
$6,000 seems low for such a severe bug across almost all Google web properties. I would expect an order of magnitude larger.
- lsaferite 7y ago
  That was my exact thought when reading the payout amount.
- tptacek 7y ago
  Why? Who else is going to outbid Google for this serverside URL parsing bug?
  - mjw1007 7y ago
    The "competition" that Google is bidding against is all the other ways that people capable of finding this sort of bug might choose to spend their free time.
    - tptacek 7y ago
      If this were Android drive-by RCE, I'd agree with you. But they're bidding for the attention of the kinds of researchers who find URL whitelist validation bugs. $6k is a strong bounty for that kind of work. (The work is important! But people who find web bugs like this tend to make their money on volume, not on a couple major hits a year, like the RCE researchers do.)
      - mygo 7y ago
        > But they're bidding for the attention of the kinds of researchers who find URL whitelist validation bugs. No they’re not. Just because this happened to be a URL whitelist bug doesn’t mean that this is the primary work that the researcher who found the bug does. Did a URL whitelist validation researcher find this, or did someone researching user auth access find this bug? If Google knew exactly who they wanted to find this bug, then Google would have already found it themselves.
terrislinenbach 7y ago
And.. it's down..
- JamesCoyne 7y ago
  Down for me too. Mirror. https://web.archive.org/web/20200309211517/https://bugs.xdavidhu.me/google/2020/03/08/the-unexpected-google-wide-domain-check-bypass/ https://web.archive.org/web/20200309211517/https://bugs.xdav...
rootsudo 7y ago
That regex explorer to map is pretty cool. Good idea - I know this would work on some places. I never thought of it like this.
londons_explore 7y ago
I feel like Google is not the only company parsing domains to check security origins... If a backslash can end a security origin, I am going to guess there are a lot of other vulnerable bits of code... One more reason joining and splitting strings with security impact is a bad idea.
- Someone1234 7y ago
  > One more reason joining and splitting strings with security impact is a bad idea. Doing it in a standard/shared library is more secure than bespoke code on each page, since at the very least as we witness here it can all be updated together as needed. You can add additional abstraction, but someone eventually has to do this work. Even web browsers themselves aren't immune from bugs in "joining and splitting strings with security impact" for URL comprehension. All you can really do is try not to re-write known secure code. Plus this buggy RegX was from the standard on URL parsing: https://tools.ietf.org/html/rfc3986#appendix-B https://tools.ietf.org/html/rfc3986#appendix-B
eternalban 7y ago
> I put this regex in www.debuggex.com. This is a really cool website What a great find.
- gajus 7y ago
  There is also regex101.com. It is my favourite personal. It highlights individual parts of the regex and provides human explanation, e.g. https://regex101.com/r/xKG7pE/1 https://regex101.com/r/xKG7pE/1
alanfranz 7y ago
URL parsing is a mess. The specification is poorly defined, and different libraries and/or servers handle them differently. This is one kind of attack; many SSRFs exist which leverage that weakness. The regex approach, btw, is even worse. Parse your URLs before doing things with them.
- wahern 7y ago
  IMO the spec is well defined and simple. What's broken are the browsers. Permitting a backslash as a delimiter rather obviously breaks the syntax. No surprise that security issues are introduced when the browsers don't follow an otherwise simple standard, while everybody else does. That's what makes this crazy. Generally I agree that people shouldn't abuse regular expressions for parsing security-critical data. But it just so happens that a standards-compliant URL can be trivially parsed using a naive regular expression that relies on the semantics of the gen-delimiters character set in the standard. In fact, it's really hard to break such a naive regular expression even when we move beyond the domain of standards compliance. One glaring exception is anything that introduces ambiguity between the userinfo authority section and the path, which the browsers have chosen to do. sigh A new RFC should be issued that updates the syntax to include the semantics which browsers give to backslash. Among other things, this will help to publicize the issue.
  - rndgermandude 7y ago
    The browsers are implementing the WhatWG URL spec, not the RFC, and it mentions \ is valid for "special" URLs (scheme is http/https, ftp, file, ws/wss): https://url.spec.whatwg.org/#hostname-state https://url.spec.whatwg.org/#hostname-state
    - robocat 7y ago
      There’s some comedy in that URL standard: Calculating file origin: "Unfortunate as it is, this is left as an exercise to the reader.” 3.7. Host equivalence: “To determine whether a host A equals B, return true if A is B, and false otherwise.” “The application/x-www-form-urlencoded format is in many ways an aberrant monstrosity”
ck2 7y ago
Such a relief some really smart and persistent people are not evil. Though you have to wonder, if there wasn't a bounty...
ultrablack 7y ago
Well done!
tialaramex 7y ago
Windows again. "Let's use a backslash instead of a slash in paths" has to be up there with "NUL-terminated strings" in terms of tiny bad ideas from long ago that are still haunting us. Of course it's not as though Google had a choice at the outset, I'm sure some non-trivial number of web sites magically didn't work (and maybe still don't work) if you insist that https://www.example.com\works\fine\on\my\pc https://www.example.com\works\fine\on\my\pc doesn't do what the person writing it expects... I think we can blame some early Internet Explorer build for first having this work (I can't see Netscape allowing it) and then it becomes a choice whether to be a purist or sigh and move on once IE gets popular and real people do it.
- catblast 7y ago
  The story goes that Microsoft advocated for a UNIX like path separator for DOS, and it was probably IBM that pushed for the backslash so that they wouldn’t be confused with the option switch character. In any event, blaming Windows for this is a bit silly.
- roywiggins 7y ago
  Oh, I still hear ads giving URLs with inexplicable backslashes in them. "Visit our website at foo.com backslash baz"
  - anschwa 7y ago
    I think few people know the difference, so they say backslash when they mean forward slash.
    - dmurray 7y ago
      The wrong way round always seemed more logical to me: you write either slash from top to bottom, so for a forward slash your pen moves backwards and vice versa.
      - Thorrez 7y ago
        The way I remember it is to think of the slash as a little person facing to the right (the direction that text flows). Is the person leaning forward or backward?
lioeters 7y ago
Many joyous moments of genuine positive hackery. Notable for me were: - At several points, unusual things were done "for some reason", following an intuition. - The one character that does the trick was discovered by brute-forcing the browser's URL parsing logic. - After finding that this little trick works, other related Google apps were explored (for "some reason") to see if the trick is generally applicable, which indeed it was. - Deep diving into Chromium source to confirm why the trick works. - How nicely the ends are tied up at the end, seeing the result of his work have an effect deep in a Closure library commonly used at Google. --- Loved this part (my emphasis): > I was always “afraid” to dig this deep, and would never have thought about looking into the source code of a browser, but after browsing around a bit, you realise that this code is also just code, and you can understand how it works. This is super and interesting and can be really helpful in many situations.
robocat 7y ago
The impact of this bug is probably very wide including server-side - not just a Google or Closure because as Someone1234 said: this buggy RegX was from the [RFC3986] standard on URL parsing: https://tools.ietf.org/html/rfc3986#appendix-B https://tools.ietf.org/html/rfc3986#appendix-B Edit 3: tl;dr: [/?#\] in browsers (including backslash) ends the host/authority but the RFC only specifies [/?#] as ending characters, so for example https://xdavidhu.me\.corp.google.com/blah https://xdavidhu.me\.corp.google.com/blah is recognised as host `xdavidhu.me` by the browser, and host `xdavidhu.me\.corp.google.com` by the RFC - so host name whitelisting or matching can be borked for security purposes. Edit: maybe do a search of github for this Regexp, and see what significant finds there are. Certainly bad actors are already doing so... Edit 2: presumably there will be a gold-rush to find other bug bounties for the same underlying issue in other software or services! Also, Google’s $6000 payout seems extremely disappointing: Google should payout on the total impact of the bug that they measure (not just the one place he happened to find). Why should the reporter have to investigate the total impact of the bug on Google in order to get a fair payout?
- deleted 7y ago
  [deleted]
- wglb 7y ago
  >Google should payout on the total impact of the bug that they measure And what is that total impact?
  - robocat 7y ago
    As said: only Google has the resources and motivation to measure the potential impact. David already identified a few places the bug might affect: “I found this regex in the Google Cloud Console’s JS, Google Actions Console’s JS, in YouTube Studio, in myaccount.google.com (!) and even in some Google Android Apps. A day later I even found this line in the Google Corp Login Page (login.corp.google.com)”.
    - wglb 7y ago
      >only Google has the resources and motivation to measure the potential impact. And apparently they have judged the impact less than folks who think $6000 is too small.
maps7 7y ago
Thanks for the write up. Really interesting and approachable for non-security people.
megous 7y ago
Again, whitelist vs blacklist. Blacklist loses. blacklist: [^/?#]+ whitelist: [a-z](?:[0-9a-z]|-(?!(?:$|\.)))*(?:\.[a-z](?:[0-9a-z]|-(?!(?:$|\.)))*)*
- robocat 7y ago
  Your whitelist loses in this situation? It accepts local domains (e.g. ‘a’) but not fully qualified domain names (e.g. ‘myhost.example.com.’ nor IP addresses. Generally when checking origin, you also wish to check port (e.g. :8000) which the Google code works for; and @username:password may need thought for some mostly obsolete purposes. That said, the whitelist does avoid a bunch of other problems (control chars, Unicode, high bit, etc).
  - megous 7y ago
    It was not meant to be complete, just for the domain part. Just like the blacklist regexp was extracted from the domain part too. Local domains are ok.
  - robocat 7y ago
    Also, your whitelist is vulnerable to redos DoS attack - according to http://redos-checker.surge.sh/ http://redos-checker.surge.sh/ I’m just trying to point out that securing via Regexps is never simple, even for the very skilled. If someone posts a comment with a Regexp, it is often broken and one can usually find problems with it. Edit: and what are the $ in there for? They look redundant.
    - megous 7y ago
      This is not meant as a complete solution for all the worlds ills, it's just an illustration of the concept and how proper matching would avoid the issue in the OP. Adding \ to the exclusion list just plasters over the pile of garbage that is the original "magic" regexp. $ are there because you can't have '-' at the end of the label. You'd need more chars there if you want to use this as a part of full URL parser. Or just use browser's facilities for parsing URLs. Also noone cares if you can DoS your own browser tab/iframe. You probably can't anyway, because browser has other mechanisms to prevent that. But yes, you'd need to limit this too in real code, to the max length of the label part of the domain name.
deathanatos 7y ago
One of these days, I'm building an integration test suite of all the weird corner cases in HTTP and whatnot to throw at libraries. God help the poor engineer here. Let's say he or she (reasonably!) looked up the grammar for a URI in RFC 3986[1], the standard that defines them. Well, according to that grammar, the browser is wrong: backslash isn't valid in that position, and the exploiting string isn't a valid production of the URI grammar. But of course, the browser has its own standard — the WHATWG URL spec — which doesn't even define a grammar for a URL, but instead defines a program in pseudocode to parse them. Normally, \ isn't valid after an authority … unless the scheme is http or https or one of several other Chosen One schemes.[2] Uuugh. We've wrought this upon ourselves. Also included in this test suite would be the wild and wonderful things you can type into a browser's URL bar that result in malformed HTTP requests hitting the wire… AFAIK, WHATWG doesn't yet have their own copy of HTTP… [1]: https://tools.ietf.org/html/rfc3986#section-3 https://tools.ietf.org/html/rfc3986#section-3 [2]: https://url.spec.whatwg.org/#hostname-state https://url.spec.whatwg.org/#hostname-state Maybe one should argue that regex should not have accepted the URI, since it's isn't valid according to the RFC.
- tal8d 7y ago
  I wrote 90% of a strict URI parser over a year ago, I know that RFC way better than I'd like to. I stopped at 90% after discovering the reason why my fuzzer was generating test failures: the ABNF is wrong. I believe it was around the query portion of URI segmentation, where two potential interpretations shared the same order of precedence and there was no rule on the order of execution. I'll finish that project one day... because I still don't know how scared I should be about that sort of ambiguity making it through more than one ABNF parser.
- chrismorgan 7y ago
  Grammars are overrated—they regularly leave ambiguities and sometimes conflict with the text, despite both being considered normative. I love WHATWG specs with their state machines. They’re all about defining exact behaviour based upon what has already been implemented by extant software, and define comprehensively how every possible input should be parsed. They’re easy to transcribe in code without error. These things get you compatibility, and avoid security bugs where people have implemented different behaviour in different places—as was the case in this instance. As a parser spec, the HTML spec is my favourite, because it makes implementing something that is fiendishly complex genuinely easy. I would love something equivalent for HTTP/1 and MIME messages.
fulafel 7y ago
This seems pretty brittle. Did anyone look into Closure to see what the motivation for the regexp based checking is? There's a browser native URL splitting function available to JS after all. But I guess Closure dates from the IE6 era where trusting browsers for correctness was probably not a good idea..
- cesarb 7y ago
  > There's a browser native URL splitting function available to JS after all. It's the URL object (https://developer.mozilla.org/en-US/docs/Web/API/URL https://developer.mozilla.org/en-US/docs/Web/API/URL), isn't it? We tried using it at work... and found out that, as always seems to be the case, it's available everywhere except on Internet Explorer.
justlexi93 7y ago
It looks like regex originally came from Appendix B of RFC 3986 so this type of vulnerability may exist in other places. According to the RFC, the regex is "for breaking-down a well-formed URI reference into its components." In this case, the URL was invalid because the host component cannot contain a backslash unless percent encoded.
dana321 7y ago
Its my opinion that when a regex gets this complicated it should be hand-coded as a state machine parser.
- Terretta 7y ago
  Complicated != complex ... it’s a straightforward expression. My experience is regex seems like line noise to people. If you show them the multi line commented form they get it, and can then appreciate the one liners. I’ve thought a neat IDE trick would be doing that expansion, into this visualization followed by multi line text you edit, and a column to paste sample text into and see matching / back-refs / captures happen. Stays one liner in code. Standalone tools do it, slicker if inline.
- qtplatypus 7y ago
  State machine parsers are more difficult for most people to understand.

644 points by notRobot 7y ago

Something something regex and two problems. \s Seriously though, nice find! It's often surprising how tiny little curiosities can lead down the rabbit hole to such interesting problems. In this case, it was a fairly logical flawed assumption, but the mess of regex made it just hidden enough to be tough to spot.

jcims 7y ago
This is one area where bug bounty programs can really outperform an internal security function. There are others, generally, but this deep rabbit hole exploration is one of the best.
hinkley 7y ago
> \s Too soon.

kyle-rb 7y ago

I love that the fix is simply a quadruple-backslash (aka an "actual backslash, for real this time" per https://xkcd.com/1638/ https://xkcd.com/1638/)

gulbrandr 7y ago
Thanks for the xkcd link, I loved it!
hinkley 7y ago
More than a couple of times have I chosen a different solution to a problem (especially shell scripts) to avoid having to double backslashes more than once. Because fuck trying to explain how that code actually works to anybody else, or myself a year from now.

chatmasta 7y ago

This was a really enjoyable, thorough and readable write-up. Thanks, and nice work!

tanseydavid 7y ago
This is the exact comment I would have written if @chatmasta had not already posted it. Thanks for the write-up.
hnick 7y ago
I'll add that I usually have trouble following these, but it was a breeze here. Glad I read it.

yvoschaap 7y ago

Great find! True hacker news worthy.

ipsum2 7y ago

$6,000 seems low for such a severe bug across almost all Google web properties. I would expect an order of magnitude larger.

lsaferite 7y ago
That was my exact thought when reading the payout amount.
tptacek 7y ago
Why? Who else is going to outbid Google for this serverside URL parsing bug?
- mjw1007 7y ago
  The "competition" that Google is bidding against is all the other ways that people capable of finding this sort of bug might choose to spend their free time.
  - tptacek 7y ago
    If this were Android drive-by RCE, I'd agree with you. But they're bidding for the attention of the kinds of researchers who find URL whitelist validation bugs. $6k is a strong bounty for that kind of work. (The work is important! But people who find web bugs like this tend to make their money on volume, not on a couple major hits a year, like the RCE researchers do.)
    - mygo 7y ago
      > But they're bidding for the attention of the kinds of researchers who find URL whitelist validation bugs. No they’re not. Just because this happened to be a URL whitelist bug doesn’t mean that this is the primary work that the researcher who found the bug does. Did a URL whitelist validation researcher find this, or did someone researching user auth access find this bug? If Google knew exactly who they wanted to find this bug, then Google would have already found it themselves.

terrislinenbach 7y ago

And.. it's down..

JamesCoyne 7y ago
Down for me too. Mirror. https://web.archive.org/web/20200309211517/https://bugs.xdavidhu.me/google/2020/03/08/the-unexpected-google-wide-domain-check-bypass/ https://web.archive.org/web/20200309211517/https://bugs.xdav...

rootsudo 7y ago

That regex explorer to map is pretty cool. Good idea - I know this would work on some places. I never thought of it like this.

londons_explore 7y ago

I feel like Google is not the only company parsing domains to check security origins... If a backslash can end a security origin, I am going to guess there are a lot of other vulnerable bits of code... One more reason joining and splitting strings with security impact is a bad idea.

Someone1234 7y ago
> One more reason joining and splitting strings with security impact is a bad idea. Doing it in a standard/shared library is more secure than bespoke code on each page, since at the very least as we witness here it can all be updated together as needed. You can add additional abstraction, but someone eventually has to do this work. Even web browsers themselves aren't immune from bugs in "joining and splitting strings with security impact" for URL comprehension. All you can really do is try not to re-write known secure code. Plus this buggy RegX was from the standard on URL parsing: https://tools.ietf.org/html/rfc3986#appendix-B https://tools.ietf.org/html/rfc3986#appendix-B

eternalban 7y ago

> I put this regex in www.debuggex.com. This is a really cool website What a great find.

gajus 7y ago
There is also regex101.com. It is my favourite personal. It highlights individual parts of the regex and provides human explanation, e.g. https://regex101.com/r/xKG7pE/1 https://regex101.com/r/xKG7pE/1

alanfranz 7y ago

URL parsing is a mess. The specification is poorly defined, and different libraries and/or servers handle them differently. This is one kind of attack; many SSRFs exist which leverage that weakness. The regex approach, btw, is even worse. Parse your URLs before doing things with them.

wahern 7y ago
IMO the spec is well defined and simple. What's broken are the browsers. Permitting a backslash as a delimiter rather obviously breaks the syntax. No surprise that security issues are introduced when the browsers don't follow an otherwise simple standard, while everybody else does. That's what makes this crazy. Generally I agree that people shouldn't abuse regular expressions for parsing security-critical data. But it just so happens that a standards-compliant URL can be trivially parsed using a naive regular expression that relies on the semantics of the gen-delimiters character set in the standard. In fact, it's really hard to break such a naive regular expression even when we move beyond the domain of standards compliance. One glaring exception is anything that introduces ambiguity between the userinfo authority section and the path, which the browsers have chosen to do. sigh A new RFC should be issued that updates the syntax to include the semantics which browsers give to backslash. Among other things, this will help to publicize the issue.
- rndgermandude 7y ago
  The browsers are implementing the WhatWG URL spec, not the RFC, and it mentions \ is valid for "special" URLs (scheme is http/https, ftp, file, ws/wss): https://url.spec.whatwg.org/#hostname-state https://url.spec.whatwg.org/#hostname-state
  - robocat 7y ago
    There’s some comedy in that URL standard: Calculating file origin: "Unfortunate as it is, this is left as an exercise to the reader.” 3.7. Host equivalence: “To determine whether a host A equals B, return true if A is B, and false otherwise.” “The application/x-www-form-urlencoded format is in many ways an aberrant monstrosity”

ck2 7y ago

Such a relief some really smart and persistent people are not evil. Though you have to wonder, if there wasn't a bounty...

ultrablack 7y ago

Well done!

tialaramex 7y ago

Windows again. "Let's use a backslash instead of a slash in paths" has to be up there with "NUL-terminated strings" in terms of tiny bad ideas from long ago that are still haunting us. Of course it's not as though Google had a choice at the outset, I'm sure some non-trivial number of web sites magically didn't work (and maybe still don't work) if you insist that https://www.example.com\works\fine\on\my\pc https://www.example.com\works\fine\on\my\pc doesn't do what the person writing it expects... I think we can blame some early Internet Explorer build for first having this work (I can't see Netscape allowing it) and then it becomes a choice whether to be a purist or sigh and move on once IE gets popular and real people do it.

catblast 7y ago
The story goes that Microsoft advocated for a UNIX like path separator for DOS, and it was probably IBM that pushed for the backslash so that they wouldn’t be confused with the option switch character. In any event, blaming Windows for this is a bit silly.
roywiggins 7y ago
Oh, I still hear ads giving URLs with inexplicable backslashes in them. "Visit our website at foo.com backslash baz"
- anschwa 7y ago
  I think few people know the difference, so they say backslash when they mean forward slash.
  - dmurray 7y ago
    The wrong way round always seemed more logical to me: you write either slash from top to bottom, so for a forward slash your pen moves backwards and vice versa.
    - Thorrez 7y ago
      The way I remember it is to think of the slash as a little person facing to the right (the direction that text flows). Is the person leaning forward or backward?

lioeters 7y ago

Many joyous moments of genuine positive hackery. Notable for me were: - At several points, unusual things were done "for some reason", following an intuition. - The one character that does the trick was discovered by brute-forcing the browser's URL parsing logic. - After finding that this little trick works, other related Google apps were explored (for "some reason") to see if the trick is generally applicable, which indeed it was. - Deep diving into Chromium source to confirm why the trick works. - How nicely the ends are tied up at the end, seeing the result of his work have an effect deep in a Closure library commonly used at Google. --- Loved this part (my emphasis): > I was always “afraid” to dig this deep, and would never have thought about looking into the source code of a browser, but after browsing around a bit, you realise that this code is also just code, and you can understand how it works. This is super and interesting and can be really helpful in many situations.

robocat 7y ago

The impact of this bug is probably very wide including server-side - not just a Google or Closure because as Someone1234 said: this buggy RegX was from the [RFC3986] standard on URL parsing: https://tools.ietf.org/html/rfc3986#appendix-B https://tools.ietf.org/html/rfc3986#appendix-B Edit 3: tl;dr: [/?#\] in browsers (including backslash) ends the host/authority but the RFC only specifies [/?#] as ending characters, so for example https://xdavidhu.me\.corp.google.com/blah https://xdavidhu.me\.corp.google.com/blah is recognised as host `xdavidhu.me` by the browser, and host `xdavidhu.me\.corp.google.com` by the RFC - so host name whitelisting or matching can be borked for security purposes. Edit: maybe do a search of github for this Regexp, and see what significant finds there are. Certainly bad actors are already doing so... Edit 2: presumably there will be a gold-rush to find other bug bounties for the same underlying issue in other software or services! Also, Google’s $6000 payout seems extremely disappointing: Google should payout on the total impact of the bug that they measure (not just the one place he happened to find). Why should the reporter have to investigate the total impact of the bug on Google in order to get a fair payout?

deleted 7y ago
[deleted]
wglb 7y ago
>Google should payout on the total impact of the bug that they measure And what is that total impact?
- robocat 7y ago
  As said: only Google has the resources and motivation to measure the potential impact. David already identified a few places the bug might affect: “I found this regex in the Google Cloud Console’s JS, Google Actions Console’s JS, in YouTube Studio, in myaccount.google.com (!) and even in some Google Android Apps. A day later I even found this line in the Google Corp Login Page (login.corp.google.com)”.
  - wglb 7y ago
    >only Google has the resources and motivation to measure the potential impact. And apparently they have judged the impact less than folks who think $6000 is too small.

maps7 7y ago

Thanks for the write up. Really interesting and approachable for non-security people.

megous 7y ago

Again, whitelist vs blacklist. Blacklist loses. blacklist: [^/?#]+ whitelist: [a-z](?:[0-9a-z]|-(?!(?:$|\.)))*(?:\.[a-z](?:[0-9a-z]|-(?!(?:$|\.)))*)*

robocat 7y ago
Your whitelist loses in this situation? It accepts local domains (e.g. ‘a’) but not fully qualified domain names (e.g. ‘myhost.example.com.’ nor IP addresses. Generally when checking origin, you also wish to check port (e.g. :8000) which the Google code works for; and @username:password may need thought for some mostly obsolete purposes. That said, the whitelist does avoid a bunch of other problems (control chars, Unicode, high bit, etc).
- megous 7y ago
  It was not meant to be complete, just for the domain part. Just like the blacklist regexp was extracted from the domain part too. Local domains are ok.
- robocat 7y ago
  Also, your whitelist is vulnerable to redos DoS attack - according to http://redos-checker.surge.sh/ http://redos-checker.surge.sh/ I’m just trying to point out that securing via Regexps is never simple, even for the very skilled. If someone posts a comment with a Regexp, it is often broken and one can usually find problems with it. Edit: and what are the $ in there for? They look redundant.
  - megous 7y ago
    This is not meant as a complete solution for all the worlds ills, it's just an illustration of the concept and how proper matching would avoid the issue in the OP. Adding \ to the exclusion list just plasters over the pile of garbage that is the original "magic" regexp. $ are there because you can't have '-' at the end of the label. You'd need more chars there if you want to use this as a part of full URL parser. Or just use browser's facilities for parsing URLs. Also noone cares if you can DoS your own browser tab/iframe. You probably can't anyway, because browser has other mechanisms to prevent that. But yes, you'd need to limit this too in real code, to the max length of the label part of the domain name.

deathanatos 7y ago

One of these days, I'm building an integration test suite of all the weird corner cases in HTTP and whatnot to throw at libraries. God help the poor engineer here. Let's say he or she (reasonably!) looked up the grammar for a URI in RFC 3986[1], the standard that defines them. Well, according to that grammar, the browser is wrong: backslash isn't valid in that position, and the exploiting string isn't a valid production of the URI grammar. But of course, the browser has its own standard — the WHATWG URL spec — which doesn't even define a grammar for a URL, but instead defines a program in pseudocode to parse them. Normally, \ isn't valid after an authority … unless the scheme is http or https or one of several other Chosen One schemes.[2] Uuugh. We've wrought this upon ourselves. Also included in this test suite would be the wild and wonderful things you can type into a browser's URL bar that result in malformed HTTP requests hitting the wire… AFAIK, WHATWG doesn't yet have their own copy of HTTP… [1]: https://tools.ietf.org/html/rfc3986#section-3 https://tools.ietf.org/html/rfc3986#section-3 [2]: https://url.spec.whatwg.org/#hostname-state https://url.spec.whatwg.org/#hostname-state Maybe one should argue that regex should not have accepted the URI, since it's isn't valid according to the RFC.

tal8d 7y ago
I wrote 90% of a strict URI parser over a year ago, I know that RFC way better than I'd like to. I stopped at 90% after discovering the reason why my fuzzer was generating test failures: the ABNF is wrong. I believe it was around the query portion of URI segmentation, where two potential interpretations shared the same order of precedence and there was no rule on the order of execution. I'll finish that project one day... because I still don't know how scared I should be about that sort of ambiguity making it through more than one ABNF parser.
chrismorgan 7y ago
Grammars are overrated—they regularly leave ambiguities and sometimes conflict with the text, despite both being considered normative. I love WHATWG specs with their state machines. They’re all about defining exact behaviour based upon what has already been implemented by extant software, and define comprehensively how every possible input should be parsed. They’re easy to transcribe in code without error. These things get you compatibility, and avoid security bugs where people have implemented different behaviour in different places—as was the case in this instance. As a parser spec, the HTML spec is my favourite, because it makes implementing something that is fiendishly complex genuinely easy. I would love something equivalent for HTTP/1 and MIME messages.

fulafel 7y ago

This seems pretty brittle. Did anyone look into Closure to see what the motivation for the regexp based checking is? There's a browser native URL splitting function available to JS after all. But I guess Closure dates from the IE6 era where trusting browsers for correctness was probably not a good idea..

cesarb 7y ago
> There's a browser native URL splitting function available to JS after all. It's the URL object (https://developer.mozilla.org/en-US/docs/Web/API/URL https://developer.mozilla.org/en-US/docs/Web/API/URL), isn't it? We tried using it at work... and found out that, as always seems to be the case, it's available everywhere except on Internet Explorer.

justlexi93 7y ago

It looks like regex originally came from Appendix B of RFC 3986 so this type of vulnerability may exist in other places. According to the RFC, the regex is "for breaking-down a well-formed URI reference into its components." In this case, the URL was invalid because the host component cannot contain a backslash unless percent encoded.

dana321 7y ago

Its my opinion that when a regex gets this complicated it should be hand-coded as a state machine parser.

Terretta 7y ago
Complicated != complex ... it’s a straightforward expression. My experience is regex seems like line noise to people. If you show them the multi line commented form they get it, and can then appreciate the one liners. I’ve thought a neat IDE trick would be doing that expansion, into this visualization followed by multi line text you edit, and a column to paste sample text into and see matching / back-refs / captures happen. Stays one liner in code. Standalone tools do it, slicker if inline.
qtplatypus 7y ago
State machine parsers are more difficult for most people to understand.