8 ms·

It's wild that we're in 2024, and there's a security issue that is essentially "we parse data in an ad-hoc format with an ad-hoc parser based on regexps, and we

by hannob 3y ago

It's wild that we're in 2024, and there's a security issue that is essentially "we parse data in an ad-hoc format with an ad-hoc parser based on regexps, and we got that wrong". And the solution is "we tweaked the regexp", and not "we properly specified/documented the format (or replaced it with something properly specified), and wrote a parser that correctly implements the specification".

meandmycode 3y ago
Especially wild for GitHub, I would have thought they had the entirety of 'git foo' codified up with actual parsers by now, but I guess a regex will always do..
- formerly_proven 3y ago
  Well the user-git-facing parts of GitHub are built on libgit2 and libssh.
- hedora 3y ago
  This doesn’t surprise me at all coming from github. I’ve tried using their public api’s, and they’re all shockingly bad. In some cases I thought it was my mistake, but then I checked their website, and found it was bug for bug compatible with the thing I’d built. My usual rule of thumb for choosing products is to go with whatever rising alternative there is to the entrenched monopoly. The little one needs to be much better to overcome monopoly effects. I don’t sign my own paycheck, but would be moving to gitlab or something if I did.
sanjayjc 3y ago
I must be missing something because the corrected regex (/\Aauthor ([^<]*)[ ]{0,1}<(.+)>/) seems to behave the same as the problematic one: https://regexr.com/7qrum I wonder whether github may not be using PCRE.
- jwilk 3y ago
  The problem wasn't that the regexp matched the second "author" line, but that it didn't match the first one.
TravisCooper 3y ago
This is the correct take.
theamk 3y ago
Even if I had to use regex, I'd at least do it in 2 stages: - Extract all "author" lines, fail-fast if there is more than one - Parse the line, again fail-fast if there is anything strange this would just stop this, and similar, attacks completely. Postel's law has no place in modern development if we want things to be secure.
- HideousKojima 3y ago
  >Postel's law Also known as "make other people's incompetence and inability to conform to a spec your problem."
  - avgcorrection 3y ago
    Yeah that “law” makes absolutely zero sense to me. It’s like “be nice” but in a self-sacrificial way which then indirectly sacrifices other people in the long run.
    - cryptonector 3y ago
      It's from an era when interoperability in a then-very-small universe was very important to moving the Internet forward. Back then there were no spammers, and very few bad actors. It's possible that the "Postel law" helped for a while -- hard to say for sure though.
    - cortesoft 3y ago
      It makes sense for a lot of situations purely out of practicality. If a good portion of your customers use tools that break spec, your choices are to accept the brokenness or lose the customer. Take browsers, for example. A lot of people complain that browsers try to work around broken certificate setups on servers, but if they didn’t, users would just switch to browsers that did.
      - avgcorrection 3y ago
        > It makes sense for a lot of situations purely out of practicality. If a good portion of your customers use tools that break spec, your choices are to accept the brokenness or lose the customer. Practical? So the adage is just “be liberal because you have to out of practical necessity”? Are you sure that’s the history behind it?
alexjplant 3y ago
I wrote a chintzy Preact app that pulls the source for a Pokemon Red hack from Github and makes Pokemon stats, moves, type matchups, etc. searchable [1] because enough has changed from the original that I wanted a quick reference for my current playthrough. I literally wrote in the TODOs in the README Generate proper AST from asm source and ditch janky Regex parsing? because I felt guilty about doing things like const match = l.match(/\s*move\s*(\w+),\s*(\w+),\s*(\d+),\s*(\w+),\s*(\d+),\s*(\d+)\s*(.*)/) Maybe I shouldn't be so hard on myself. [1] https://github.com/alexjplant/rgbdex https://github.com/alexjplant/rgbdex
- willdr 3y ago
  A proper AST would "read" the source code programmatically, as opposed to brittle regex? Is there any benefit to building that when your regex works and what you're parsing is static? (Genuine questions, my compsci background is not strong).
js2 3y ago
There's nothing inherently wrong with using a regular expression here. The security issue is that there are two different parsers and they interpret invalid input differently. This is a parsing differential vulnerability. This security issue can (and does) occur whenever there is more than one parser implementation.
- theamk 3y ago
  git treats objects as a generic structure ("list of space-separated name-value pairs, maybe with space continuations, followed by double newline, followed my message") followed by additional rules for individual values. Something very common in older protocols -- see HTTP, deb control files, email, etc... And github code decided to short-circuit the process and combine two steps into one, which introduced weird inter-layer interactions where lines with good name but invalid content would be silently rejected.

by hannob 3y ago

meandmycode 3y ago

Especially wild for GitHub, I would have thought they had the entirety of 'git foo' codified up with actual parsers by now, but I guess a regex will always do..

formerly_proven 3y ago
Well the user-git-facing parts of GitHub are built on libgit2 and libssh.
hedora 3y ago
This doesn’t surprise me at all coming from github. I’ve tried using their public api’s, and they’re all shockingly bad. In some cases I thought it was my mistake, but then I checked their website, and found it was bug for bug compatible with the thing I’d built. My usual rule of thumb for choosing products is to go with whatever rising alternative there is to the entrenched monopoly. The little one needs to be much better to overcome monopoly effects. I don’t sign my own paycheck, but would be moving to gitlab or something if I did.

sanjayjc 3y ago

I must be missing something because the corrected regex (/\Aauthor ([^<]*)[ ]{0,1}<(.+)>/) seems to behave the same as the problematic one: https://regexr.com/7qrum I wonder whether github may not be using PCRE.

jwilk 3y ago
The problem wasn't that the regexp matched the second "author" line, but that it didn't match the first one.

TravisCooper 3y ago

This is the correct take.

theamk 3y ago

Even if I had to use regex, I'd at least do it in 2 stages: - Extract all "author" lines, fail-fast if there is more than one - Parse the line, again fail-fast if there is anything strange this would just stop this, and similar, attacks completely. Postel's law has no place in modern development if we want things to be secure.

HideousKojima 3y ago
>Postel's law Also known as "make other people's incompetence and inability to conform to a spec your problem."
- avgcorrection 3y ago
  Yeah that “law” makes absolutely zero sense to me. It’s like “be nice” but in a self-sacrificial way which then indirectly sacrifices other people in the long run.
  - cryptonector 3y ago
    It's from an era when interoperability in a then-very-small universe was very important to moving the Internet forward. Back then there were no spammers, and very few bad actors. It's possible that the "Postel law" helped for a while -- hard to say for sure though.
  - cortesoft 3y ago
    It makes sense for a lot of situations purely out of practicality. If a good portion of your customers use tools that break spec, your choices are to accept the brokenness or lose the customer. Take browsers, for example. A lot of people complain that browsers try to work around broken certificate setups on servers, but if they didn’t, users would just switch to browsers that did.
    - avgcorrection 3y ago
      > It makes sense for a lot of situations purely out of practicality. If a good portion of your customers use tools that break spec, your choices are to accept the brokenness or lose the customer. Practical? So the adage is just “be liberal because you have to out of practical necessity”? Are you sure that’s the history behind it?

alexjplant 3y ago

I wrote a chintzy Preact app that pulls the source for a Pokemon Red hack from Github and makes Pokemon stats, moves, type matchups, etc. searchable [1] because enough has changed from the original that I wanted a quick reference for my current playthrough. I literally wrote in the TODOs in the README Generate proper AST from asm source and ditch janky Regex parsing? because I felt guilty about doing things like const match = l.match(/\s*move\s*(\w+),\s*(\w+),\s*(\d+),\s*(\w+),\s*(\d+),\s*(\d+)\s*(.*)/) Maybe I shouldn't be so hard on myself. [1] https://github.com/alexjplant/rgbdex https://github.com/alexjplant/rgbdex

willdr 3y ago
A proper AST would "read" the source code programmatically, as opposed to brittle regex? Is there any benefit to building that when your regex works and what you're parsing is static? (Genuine questions, my compsci background is not strong).

js2 3y ago

There's nothing inherently wrong with using a regular expression here. The security issue is that there are two different parsers and they interpret invalid input differently. This is a parsing differential vulnerability. This security issue can (and does) occur whenever there is more than one parser implementation.

theamk 3y ago
git treats objects as a generic structure ("list of space-separated name-value pairs, maybe with space continuations, followed by double newline, followed my message") followed by additional rules for individual values. Something very common in older protocols -- see HTTP, deb control files, email, etc... And github code decided to short-circuit the process and combine two steps into one, which introduced weird inter-layer interactions where lines with good name but invalid content would be silently rejected.