11 ms·

Forging signed commits on GitHub

308 points by rokkitmensch 3y ago

Jones4000 3y ago
[dead]
YetAnotherNick 3y ago
Really nice and educational exploit. Regexes should almost never be used in parsing structured stuff in production. While it is developer friendly, it's very hard to think all the edge cases.
- _nalply 3y ago
  What I tought, they should have used some more official library to manage git instead of doing the work themselves, but perhaps there was none?
  - j16sdiz 3y ago
    We have libgit2, but github have their own ruby implementation of everything. (java and rust have their own "native" implementation too.
    - guappa 3y ago
      libgit2 was written after github. So that might explain.
      - bspammer 3y ago
        libgit2's frontpage claims that GitHub does use them: https://libgit2.org/ https://libgit2.org/
        guappa 3y ago
        Depends which part of github uses them. Might even be their GUI client…
- Zababa 3y ago
  Are there alternatives that make it easier to think about all the edge cases?
  - michaelt 3y ago
    Keep using regular expressions, just stop using . and instead, define what is allowed. You're expecting an input to be a UUID? Check it with ^[a-fA-F0-9\-]{30,40}$ and you know you're not getting any apostrophes or script tags or enormous inputs or empty inputs or newlines or lookalike characters or emojis.
    - yencabulator 3y ago
      The fault is still present in that, you shouldn't just regexp "look for needle in haystack" in a single phase when you're supposed to extract key-value pairs and operate on the author key. What happened here was that the Github employee programming this didn't bother to read the spec and/or think.
  - ylk 3y ago
    I think re-implementing the functionality is the mistake here. A big counter-argument to “rewrite in rust” is usually that by rewriting you introduce new bugs. Especially for security critical things one should re-use the implementation to avoid issues like the above. Proving both the original and re-implemented parser to be equivalent would probably also work, but not sure how practical. And in any case have competent people audit what you did.
    - avgcorrection 3y ago
      How did we get from criticizing regexes to mocking “rewrite in rust” in the span of three comments?
      - ylk 3y ago
        I’m not mocking it. I’m just applying the argument I see many people on HN make in discussions about rust to this case, where I suspect many will lean much more on the side of using regexes to reimplement parsers. Don’t know if that’ll be the same people. Either way, I didn’t want to have a discussion about rust.
- akdor1154 3y ago
  Disagree here, the problem is their implementation was slightly different to Git's. There is more chance of stuffing up like this with a handrolled parser than with a regex. The only viable alternative i'd accept here to actually reduce risk is to call into libgit itself. Absent that, a regex is appropriate for this problem. There are many problems where regexes are succinct and appropriate, (of course there are many more where they are neither).
  - YetAnotherNick 3y ago
    Unless git uses regex, it is almost impossible to verify that the regex is same as their validation. What if git's validation changes. Even looking at the corrected regex I am not sure what would happen for strings like '<a@a.com>>' or '<' or 'x <a>'. Not saying the github's corrected regex is wrong, but it is a mental challenge to verify it.
- thrdbndndn 3y ago
  When the data itself is just plain text, are there any other (better) ways?
  - brokensegue 3y ago
    Parsers?
    - davemp 3y ago
      I don't see how writing a parser from scratch would mitigate bugs vs using a regex parser. Parsers are just a hot spot for security bugs that should get extra scrutiny.
    - semanticc 3y ago
      Regex is the parser for regular languages.
- cedilla 3y ago
  This isn't really a regex problem though. The regex probably did exactly what the dev intended, it's just that the dev failed to take into account that <author> could also be a 0 length string. The same logic bug would have occured with a parser.
  - account42 3y ago
    This is all speculation of course but a parser is more likely to recognize the author: prefix and then error out on anything unexpected in the rest of the line. Meanwhile regular expressions don't have any way to report errors.
    - glenjamin 3y ago
      Personally I’d have separated finding the Author line from pulling out the contents of that line - but still used Regex
notso411 3y ago
[dead]
Vogtinator 3y ago
I treat "signed by GitHub's verified signature" equivalent to unsigned. It's not the author's signature, just some third party's.
- sureglymop 3y ago
  One should treat git itself as insecure. When you sign a commit, the signature is based on the commit message and commit metadata including tree object_id and parent object_id. But if sha1 is used to generate commit hashes, one can theoretically forge a commit. This means that in an elaborate supply chain attack, one could spoof a commit with a valid signature. That signature would then still appear valid for the spoofed commit and probably make it seem more legitimate than it is.
  - Anunayj 3y ago
    For people who might be wondering why git hasn't moved to sha256 yet, here's a lwn article on it: https://lwn.net/Articles/898522/ https://lwn.net/Articles/898522/
    - CuriousCosmic 3y ago
      For a more recent bit of info on it (I remembered this thread from the latter half of last year on the mailing list): https://lore.kernel.org/git/2f5de416-04ba-c23d-1e0b-83bb655829a7@zombino.com/t/#u https://lore.kernel.org/git/2f5de416-04ba-c23d-1e0b-83bb6558... The important snippet from this thread is: > On 6/29/23 07:59, Junio C Hamano wrote: > > Adam Majer <adamm@zombino.com> writes: > > > Is sha256 still considered experimental or can it be assumed to be stable? > > I do not think we would officially label SHA-256 support as "stable" until we have good interoperability with SHA-1 repositories, but the expectation is that we will make reasonable effort to keep migration path for the current SHA-256 repositories, even if it turns out that its on-disk format need to be updated, to keep the end-user data safe. > That could be a different definition of stable. But I'm satisfied that current sha256 repositories will not end up incompatible with some future version of git without migration path (talking about on-disk format). > So maybe my question should be reworded to "is sha256 still considered early stage, for testing purposes only with possible data-loss or can it be relied on for actual long lived repositories?" > > So while "no-longer-experimental" patch is probably a bit premature, the warning in flashing red letters to caution against any use other than testing may want to be toned down. > Agreed. I think it should be clear that SHA256 and SHA1 repositories cannot share data at this point. The scary wording should be removed though, as currently it sounds like "data loss incoming and it's your fault" if one chooses sha256 > - Adam TLDR: SHA256 works well and can be considered "beta". It's viable but you can't really share data between a SHA1 and SHA256 repo. So if your repo doesn't need to merge to/from existing sha1 repos and you aren't dealing with subtrees or anything fancy like that, you should be able to use SHA256. It is not expected to break your repo, it's reasonably well supported, and in the event a migration needs to occur, there will be support for that as well. So if you want to jump on the SHA256 train you absolutely can provided you are following a pretty normal/boring use case.
photoGrant 3y ago
> April 24 2023: GitHub closes the issue, saying that being able to impersonate your own account is not an issue I mean, fear & curiosity would have me testing it before closing. Wha?
- michaelt 3y ago
  As I understand things, everyone who runs a bug bounty gets an endless stream of useless reports telling them things like "your website sends the server header which makes you vulnerable to hacking, bounty please" Therefore, they have very junior people assigned to triage bug reports. And those very junior people are in the habit of closing 99.9% of reports as not-really-a-security-problem.
  - dns_snek 3y ago
    Anecdote time, I discovered a bug where a bank app would continue receiving 2FA notification prompts even after the user terminated the session from another device (e.g. after realizing their device was stolen). The app was apparently still holding onto some kind of a valid token that could be used to approve 2FA requests despite being "logged out". I submitted detailed reproduction steps, with a video clearly showing the bug. Triage claimed that this was "intended behavior" and as far as I'm aware, the bank didn't even see my report.
    - zero_k 3y ago
      Haha yes, that's the norm. They don't care. Report to the regulator. Then they care. Find out who the regulator is in your country, and report to them. Kinda annoying, but when the regulator tells them they need to do it, it magically gets done. Note that the regulator doesn't threaten with childish things like a fine of 1M USD, that's kids' playground stuff. They threaten to revoke their banking license, which will cost them 100x more per day :)
    - tedivm 3y ago
      I've found that responding "Well, if that's intended behavior you don't mind if I blog about it then? The post will go up in a week." tends to help them take it more seriously.
_cenw 3y ago
GitHub closing any security issue by default sure is a thing. I reported an issue that was not fun to reproduce in their SAML implementation, where they would include organization membership in OAuth tokens even if you didn't have a SAML session, so if you did device posture in your SAML provider to protect from access to code, installing apps that allow you to get code (e.g. SonarCloud) would allow bypass. The weirdest thing is that they had an endpoint that did check if there was a SAML session (membership API would 403 without), but I still got told this wasn't a bug and within expected behavior (not for our CISO it wasn't, and it's still not documented). Shoutout to Tailscale for sending me some stuff despite it not being an issue in their implementation.
datadeft 3y ago
> GitHub fixed the issue by changing the problematic regex Why am I not surprised.
- deleted 3y ago
  [deleted]
asimpletune 3y ago
One crucial thing that enabled this work is being able to decompile the enterprise version of the GitHub binary. It makes you think that distributing a binary of your app is either a good idea or bad idea, depending on how you view security. If your app is important though then it’s definitely a good idea.
- greatgib 3y ago
  At the opposite, thanks to that they now have a safer platform. At the opposite, there could still be nefarious insiders that discover that and abuse of it or resell the exploit for years without anyone noticing. Especially for a proprietary service, as a customer, for me it is an added bonus point to know that the thing can and is battle tested by outsiders...
- epcoa 3y ago
  > [was] being able to decompile the enterprise version of the GitHub binary. > It makes you think that distributing a binary of your app is either a good idea or bad idea, > If your app is important though then it’s definitely a good idea. I don’t follow. What are you saying would have been the good idea here? It also isn’t clear to me that they required the code except to answer why it works, certainly this parsing defect could be discovered simply by testing the endpoint.
  - asimpletune 3y ago
    That distributing a binary app was a good idea. The attacker found and fixed the vulnerability. Of course it wasn't required to find the vulnerability, however if you make it then it's more likely that good guys will find and fix stuff. The contrary would be it's difficult to find this stuff, so then only highly motivated bad guys do it, because at that point 10 grand might not be worth it. Again, this was all prefaced with it depends. Some might see it as making it easier to find problems, and others might say 'yeah, exactly.". It's just an opinion. A fact is however that a binary makes it easier. Source code would be even easier.
    - 3y ago
avgcorrection 3y ago
I don’t like how GitHub changes my committer to them in the name of being Verified. I don’t care about it being Verified by GitHub (yes, even if it had always worked and not allowed spoofing). It’s weird that GitHub has such a long history of cooperation with the Git project and yet just rewriting the committer is considered okay to them.
- sp1rit 3y ago
  It's common practice to have the committer not match the author (especially in environments where only patches/diffs are sent). I can't imagine the git project having any issue with that, given that the author field stays retained (ignoring this bug ofc.).
  - avgcorrection 3y ago
    I tried to do some more research on this. Unfortunately the search combination > rewrite commit github verified is so insanely SEO-poisoned by (1) StackOverflow questions about how to rewrite commits and (2) GitHub’s docs about regular (built-in Git commit verification) commit verification. But I thought about it some more. And I guess them hijacking the committer on pull request merges is not that insanely bothersome. It’s their platform after all. Just yet another reason to not interact with forges in a way that affects Git itself. But another thing I’ve noticed is that sometimes my committer becomes the stupid initial (legacy) email I used to sign up there. Why in the hell? My current email is already registered there. But I tested this now and at least I got the option to merge as one of my addresses. So I guess it was fixed? > It's common practice to have the committer not match the author When the committer and author are different people. And apparently when GitHub tries to insert itself as a pseudo-person. I expected that my own actions would be tied to my own identity on GitHub. Not for them to shoehorn GitHub Verified™ into commit objects that I create through clickity-clacking around their UI. But it is after all their UI and they can poison commits however they like since they create it (i.e. I didn’t create it and send it to them; then I would have noticed if they tried to rewrite it). So shame on me. > especially in environments where only patches/diffs are sent Clearly doesn’t apply here. > I can't imagine the git project having any issue with that, given that the author field stays retained (ignoring this bug ofc.). Given that they don’t use GitHub for anything directly related to Git (PRs and such) and that they have their own way of indicating code provenance: No, I guess they have no reason to care. (Other than in spirit.) But it matters in general (in spirit) that if I pick up some patch by Jack Cooper and apply it then the committer is me. Not outlook.com. Or whatever program did it for me. The commit object has a few metadata fields. I’ve never seen anyone say, “Oh yeah sure, just use that field for whatever it’s not like it matters anyway”.
jaimex2 3y ago
Is there anything more pointless than signed commits? If your security team ever enforced it you're probably already pwned but may as well replace them just in case.
- samcat116 3y ago
  Why do you say that? It seems like they would be more important in the context of supply chain attacks.
  - jaimex2 3y ago
    If they were able to get your keys to push a commit or sneak code into your branch we are waaaaay past the territory where a signed commit can be trusted.
tumetab1 3y ago
(2023)
richbell 3y ago
> Since the first author name is zero characters long, the regex skips that line, and the fake second author line is used instead. Git ignores extra author lines after the first, so Codespaces looks at the second author line but Git looks at the first. How common is it to have multiple author lines? I haven't seen that before, only co-author footer[1]. [1]: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors https://docs.github.com/en/pull-requests/committing-changes-...
- avgcorrection 3y ago
  Apparently multiple authors is invalid.[1] [1] https://stackoverflow.com/questions/65445147/how-can-it-be-that-a-commit-object-has-2-authors https://stackoverflow.com/questions/65445147/how-can-it-be-t...
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.
dboreham 3y ago
If you use a regex in your application you need to write lots of unit tests for it that include many negative test cases. Perhaps even look for a test case generation tool/library that helps automate this.
- layer8 3y ago
  The better solution is to have a formal specification of the syntax, that the regular expression can straightforwardly be derived from and be compared with.
captn3m0 3y ago
I’ve always avoided blind signing endpoints for this reason. You’re reducing the authenticity guarantee by a thousandfold, and increasing so much complexity. I don’t think there’s an easier way though short of not supporting signatures in codespaces, and this definitely feels like a feature where the Codespaces product team’s roadmap was prioritised over the Security team.
- semiquaver 3y ago
  Now that SSH key signing is a thing, you might be able to use SSH agent forwarding to sign commits without having to hand over your private key.
  - namibj 3y ago
    ...GPG had such a feature for ages: it's AFAIK largely a result from very modular/flexible smartcard/HSM support.
- nightpool 3y ago
  It would be better to generate a user-and-codespaces-specific private key that would be safe to give to the user by checking out in the codespace so that you can use normal GPG signing flows.
AlwaysRock 3y ago
Signing commits is such a pain to set up. I'm convinced its such a boring area with so little profit that no one has spent enough time to make it more straight forward.
- sitzkrieg 3y ago
  you can sign w ssh key w 2 git configs set. im not sure this qualifies as pain
  - cryptonector 3y ago
    It's meaningless unless your keys are known to others to speak for you. I.e., you have to participate in the PGP trust mesh, and that is what is a pain.
    - AlwaysRock 3y ago
      Yup. This.
    - woodrowbarlow 3y ago
      isn't this what keybase is for?
    - arccy 3y ago
      or github.com/<username>.keys
    - Arnavion 3y ago
      https://datatracker.ietf.org/doc/draft-koch-openpgp-webkey-service/17/ https://datatracker.ietf.org/doc/draft-koch-openpgp-webkey-s... Serve the key at a certain URL from a webserver on your email domain. Clients that trust the domain can trust the key that it serves.
- influx 3y ago
  1Password makes it easy to do with ssh keys and biometrics. It’s actually pretty slick.
nightpool 3y ago
Why does Github use the same GPG key for all users? Feels like it makes these kind of confused deputy attacks endemic to the entire platform rather than being contained to a single user account.
gwern 3y ago
> Since the first author name is zero characters long, the regex skips that line, and the fake second author line is used instead. Git ignores extra author lines after the first, so Codespaces looks at the second author line but Git looks at the first. This means we can create GitHub-signed commits with any author name+email. Classic weird machine exploit of looking at two independent ad hoc parsers and finding inputs where they desynchronize and enter different states.
n4jm4 3y ago
Relying on regexes instead of thoroughly tested parser AST's encourages vulnerabilities. Are they really scraping logs for credentials instead of using an API?
layer8 3y ago
This looks like a classic case of a parser mismatch vulnerability [0]. [0] https://www.brainonfire.net/blog/2022/04/11/what-is-parser-mismatch/ https://www.brainonfire.net/blog/2022/04/11/what-is-parser-m...
- jwilk 3y ago
  Discussed on HN: https://news.ycombinator.com/item?id=38743029 https://news.ycombinator.com/item?id=38743029 (22 comments)
1oooqooq 3y ago
the fact they haven't published an analysis with every single case this was abused, even if zero, in the past is proof enough this was a feature to someone.

308 points by rokkitmensch 3y ago

[dead]

Really nice and educational exploit. Regexes should almost never be used in parsing structured stuff in production. While it is developer friendly, it's very hard to think all the edge cases.

_nalply 3y ago
What I tought, they should have used some more official library to manage git instead of doing the work themselves, but perhaps there was none?
- j16sdiz 3y ago
  We have libgit2, but github have their own ruby implementation of everything. (java and rust have their own "native" implementation too.
  - guappa 3y ago
    libgit2 was written after github. So that might explain.
    - bspammer 3y ago
      libgit2's frontpage claims that GitHub does use them: https://libgit2.org/ https://libgit2.org/
      - guappa 3y ago
        Depends which part of github uses them. Might even be their GUI client…
Zababa 3y ago
Are there alternatives that make it easier to think about all the edge cases?
- michaelt 3y ago
  Keep using regular expressions, just stop using . and instead, define what is allowed. You're expecting an input to be a UUID? Check it with ^[a-fA-F0-9\-]{30,40}$ and you know you're not getting any apostrophes or script tags or enormous inputs or empty inputs or newlines or lookalike characters or emojis.
  - yencabulator 3y ago
    The fault is still present in that, you shouldn't just regexp "look for needle in haystack" in a single phase when you're supposed to extract key-value pairs and operate on the author key. What happened here was that the Github employee programming this didn't bother to read the spec and/or think.
- ylk 3y ago
  I think re-implementing the functionality is the mistake here. A big counter-argument to “rewrite in rust” is usually that by rewriting you introduce new bugs. Especially for security critical things one should re-use the implementation to avoid issues like the above. Proving both the original and re-implemented parser to be equivalent would probably also work, but not sure how practical. And in any case have competent people audit what you did.
  - avgcorrection 3y ago
    How did we get from criticizing regexes to mocking “rewrite in rust” in the span of three comments?
    - ylk 3y ago
      I’m not mocking it. I’m just applying the argument I see many people on HN make in discussions about rust to this case, where I suspect many will lean much more on the side of using regexes to reimplement parsers. Don’t know if that’ll be the same people. Either way, I didn’t want to have a discussion about rust.
akdor1154 3y ago
Disagree here, the problem is their implementation was slightly different to Git's. There is more chance of stuffing up like this with a handrolled parser than with a regex. The only viable alternative i'd accept here to actually reduce risk is to call into libgit itself. Absent that, a regex is appropriate for this problem. There are many problems where regexes are succinct and appropriate, (of course there are many more where they are neither).
- YetAnotherNick 3y ago
  Unless git uses regex, it is almost impossible to verify that the regex is same as their validation. What if git's validation changes. Even looking at the corrected regex I am not sure what would happen for strings like '<a@a.com>>' or '<' or 'x <a>'. Not saying the github's corrected regex is wrong, but it is a mental challenge to verify it.
thrdbndndn 3y ago
When the data itself is just plain text, are there any other (better) ways?
- brokensegue 3y ago
  Parsers?
  - davemp 3y ago
    I don't see how writing a parser from scratch would mitigate bugs vs using a regex parser. Parsers are just a hot spot for security bugs that should get extra scrutiny.
  - semanticc 3y ago
    Regex is the parser for regular languages.
cedilla 3y ago
This isn't really a regex problem though. The regex probably did exactly what the dev intended, it's just that the dev failed to take into account that <author> could also be a 0 length string. The same logic bug would have occured with a parser.
- account42 3y ago
  This is all speculation of course but a parser is more likely to recognize the author: prefix and then error out on anything unexpected in the rest of the line. Meanwhile regular expressions don't have any way to report errors.
  - glenjamin 3y ago
    Personally I’d have separated finding the Author line from pulling out the contents of that line - but still used Regex

notso411 3y ago

[dead]

Vogtinator 3y ago

I treat "signed by GitHub's verified signature" equivalent to unsigned. It's not the author's signature, just some third party's.

sureglymop 3y ago
One should treat git itself as insecure. When you sign a commit, the signature is based on the commit message and commit metadata including tree object_id and parent object_id. But if sha1 is used to generate commit hashes, one can theoretically forge a commit. This means that in an elaborate supply chain attack, one could spoof a commit with a valid signature. That signature would then still appear valid for the spoofed commit and probably make it seem more legitimate than it is.
- Anunayj 3y ago
  For people who might be wondering why git hasn't moved to sha256 yet, here's a lwn article on it: https://lwn.net/Articles/898522/ https://lwn.net/Articles/898522/
  - CuriousCosmic 3y ago
    For a more recent bit of info on it (I remembered this thread from the latter half of last year on the mailing list): https://lore.kernel.org/git/2f5de416-04ba-c23d-1e0b-83bb655829a7@zombino.com/t/#u https://lore.kernel.org/git/2f5de416-04ba-c23d-1e0b-83bb6558... The important snippet from this thread is: > On 6/29/23 07:59, Junio C Hamano wrote: > > Adam Majer <adamm@zombino.com> writes: > > > Is sha256 still considered experimental or can it be assumed to be stable? > > I do not think we would officially label SHA-256 support as "stable" until we have good interoperability with SHA-1 repositories, but the expectation is that we will make reasonable effort to keep migration path for the current SHA-256 repositories, even if it turns out that its on-disk format need to be updated, to keep the end-user data safe. > That could be a different definition of stable. But I'm satisfied that current sha256 repositories will not end up incompatible with some future version of git without migration path (talking about on-disk format). > So maybe my question should be reworded to "is sha256 still considered early stage, for testing purposes only with possible data-loss or can it be relied on for actual long lived repositories?" > > So while "no-longer-experimental" patch is probably a bit premature, the warning in flashing red letters to caution against any use other than testing may want to be toned down. > Agreed. I think it should be clear that SHA256 and SHA1 repositories cannot share data at this point. The scary wording should be removed though, as currently it sounds like "data loss incoming and it's your fault" if one chooses sha256 > - Adam TLDR: SHA256 works well and can be considered "beta". It's viable but you can't really share data between a SHA1 and SHA256 repo. So if your repo doesn't need to merge to/from existing sha1 repos and you aren't dealing with subtrees or anything fancy like that, you should be able to use SHA256. It is not expected to break your repo, it's reasonably well supported, and in the event a migration needs to occur, there will be support for that as well. So if you want to jump on the SHA256 train you absolutely can provided you are following a pretty normal/boring use case.

photoGrant 3y ago

> April 24 2023: GitHub closes the issue, saying that being able to impersonate your own account is not an issue I mean, fear & curiosity would have me testing it before closing. Wha?

michaelt 3y ago
As I understand things, everyone who runs a bug bounty gets an endless stream of useless reports telling them things like "your website sends the server header which makes you vulnerable to hacking, bounty please" Therefore, they have very junior people assigned to triage bug reports. And those very junior people are in the habit of closing 99.9% of reports as not-really-a-security-problem.
- dns_snek 3y ago
  Anecdote time, I discovered a bug where a bank app would continue receiving 2FA notification prompts even after the user terminated the session from another device (e.g. after realizing their device was stolen). The app was apparently still holding onto some kind of a valid token that could be used to approve 2FA requests despite being "logged out". I submitted detailed reproduction steps, with a video clearly showing the bug. Triage claimed that this was "intended behavior" and as far as I'm aware, the bank didn't even see my report.
  - zero_k 3y ago
    Haha yes, that's the norm. They don't care. Report to the regulator. Then they care. Find out who the regulator is in your country, and report to them. Kinda annoying, but when the regulator tells them they need to do it, it magically gets done. Note that the regulator doesn't threaten with childish things like a fine of 1M USD, that's kids' playground stuff. They threaten to revoke their banking license, which will cost them 100x more per day :)
  - tedivm 3y ago
    I've found that responding "Well, if that's intended behavior you don't mind if I blog about it then? The post will go up in a week." tends to help them take it more seriously.

_cenw 3y ago

GitHub closing any security issue by default sure is a thing. I reported an issue that was not fun to reproduce in their SAML implementation, where they would include organization membership in OAuth tokens even if you didn't have a SAML session, so if you did device posture in your SAML provider to protect from access to code, installing apps that allow you to get code (e.g. SonarCloud) would allow bypass. The weirdest thing is that they had an endpoint that did check if there was a SAML session (membership API would 403 without), but I still got told this wasn't a bug and within expected behavior (not for our CISO it wasn't, and it's still not documented). Shoutout to Tailscale for sending me some stuff despite it not being an issue in their implementation.

datadeft 3y ago

> GitHub fixed the issue by changing the problematic regex Why am I not surprised.

deleted 3y ago
[deleted]

asimpletune 3y ago

One crucial thing that enabled this work is being able to decompile the enterprise version of the GitHub binary. It makes you think that distributing a binary of your app is either a good idea or bad idea, depending on how you view security. If your app is important though then it’s definitely a good idea.

greatgib 3y ago
At the opposite, thanks to that they now have a safer platform. At the opposite, there could still be nefarious insiders that discover that and abuse of it or resell the exploit for years without anyone noticing. Especially for a proprietary service, as a customer, for me it is an added bonus point to know that the thing can and is battle tested by outsiders...
epcoa 3y ago
> [was] being able to decompile the enterprise version of the GitHub binary. > It makes you think that distributing a binary of your app is either a good idea or bad idea, > If your app is important though then it’s definitely a good idea. I don’t follow. What are you saying would have been the good idea here? It also isn’t clear to me that they required the code except to answer why it works, certainly this parsing defect could be discovered simply by testing the endpoint.
- asimpletune 3y ago
  That distributing a binary app was a good idea. The attacker found and fixed the vulnerability. Of course it wasn't required to find the vulnerability, however if you make it then it's more likely that good guys will find and fix stuff. The contrary would be it's difficult to find this stuff, so then only highly motivated bad guys do it, because at that point 10 grand might not be worth it. Again, this was all prefaced with it depends. Some might see it as making it easier to find problems, and others might say 'yeah, exactly.". It's just an opinion. A fact is however that a binary makes it easier. Source code would be even easier.
  - 3y ago

avgcorrection 3y ago

I don’t like how GitHub changes my committer to them in the name of being Verified. I don’t care about it being Verified by GitHub (yes, even if it had always worked and not allowed spoofing). It’s weird that GitHub has such a long history of cooperation with the Git project and yet just rewriting the committer is considered okay to them.

sp1rit 3y ago
It's common practice to have the committer not match the author (especially in environments where only patches/diffs are sent). I can't imagine the git project having any issue with that, given that the author field stays retained (ignoring this bug ofc.).
- avgcorrection 3y ago
  I tried to do some more research on this. Unfortunately the search combination > rewrite commit github verified is so insanely SEO-poisoned by (1) StackOverflow questions about how to rewrite commits and (2) GitHub’s docs about regular (built-in Git commit verification) commit verification. But I thought about it some more. And I guess them hijacking the committer on pull request merges is not that insanely bothersome. It’s their platform after all. Just yet another reason to not interact with forges in a way that affects Git itself. But another thing I’ve noticed is that sometimes my committer becomes the stupid initial (legacy) email I used to sign up there. Why in the hell? My current email is already registered there. But I tested this now and at least I got the option to merge as one of my addresses. So I guess it was fixed? > It's common practice to have the committer not match the author When the committer and author are different people. And apparently when GitHub tries to insert itself as a pseudo-person. I expected that my own actions would be tied to my own identity on GitHub. Not for them to shoehorn GitHub Verified™ into commit objects that I create through clickity-clacking around their UI. But it is after all their UI and they can poison commits however they like since they create it (i.e. I didn’t create it and send it to them; then I would have noticed if they tried to rewrite it). So shame on me. > especially in environments where only patches/diffs are sent Clearly doesn’t apply here. > I can't imagine the git project having any issue with that, given that the author field stays retained (ignoring this bug ofc.). Given that they don’t use GitHub for anything directly related to Git (PRs and such) and that they have their own way of indicating code provenance: No, I guess they have no reason to care. (Other than in spirit.) But it matters in general (in spirit) that if I pick up some patch by Jack Cooper and apply it then the committer is me. Not outlook.com. Or whatever program did it for me. The commit object has a few metadata fields. I’ve never seen anyone say, “Oh yeah sure, just use that field for whatever it’s not like it matters anyway”.

jaimex2 3y ago

Is there anything more pointless than signed commits? If your security team ever enforced it you're probably already pwned but may as well replace them just in case.

samcat116 3y ago
Why do you say that? It seems like they would be more important in the context of supply chain attacks.
- jaimex2 3y ago
  If they were able to get your keys to push a commit or sneak code into your branch we are waaaaay past the territory where a signed commit can be trusted.

tumetab1 3y ago

(2023)

richbell 3y ago

> Since the first author name is zero characters long, the regex skips that line, and the fake second author line is used instead. Git ignores extra author lines after the first, so Codespaces looks at the second author line but Git looks at the first. How common is it to have multiple author lines? I haven't seen that before, only co-author footer[1]. [1]: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors https://docs.github.com/en/pull-requests/committing-changes-...

avgcorrection 3y ago
Apparently multiple authors is invalid.[1] [1] https://stackoverflow.com/questions/65445147/how-can-it-be-that-a-commit-object-has-2-authors https://stackoverflow.com/questions/65445147/how-can-it-be-t...

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.

dboreham 3y ago

If you use a regex in your application you need to write lots of unit tests for it that include many negative test cases. Perhaps even look for a test case generation tool/library that helps automate this.

layer8 3y ago
The better solution is to have a formal specification of the syntax, that the regular expression can straightforwardly be derived from and be compared with.

captn3m0 3y ago

I’ve always avoided blind signing endpoints for this reason. You’re reducing the authenticity guarantee by a thousandfold, and increasing so much complexity. I don’t think there’s an easier way though short of not supporting signatures in codespaces, and this definitely feels like a feature where the Codespaces product team’s roadmap was prioritised over the Security team.

semiquaver 3y ago
Now that SSH key signing is a thing, you might be able to use SSH agent forwarding to sign commits without having to hand over your private key.
- namibj 3y ago
  ...GPG had such a feature for ages: it's AFAIK largely a result from very modular/flexible smartcard/HSM support.
nightpool 3y ago
It would be better to generate a user-and-codespaces-specific private key that would be safe to give to the user by checking out in the codespace so that you can use normal GPG signing flows.

AlwaysRock 3y ago

Signing commits is such a pain to set up. I'm convinced its such a boring area with so little profit that no one has spent enough time to make it more straight forward.

sitzkrieg 3y ago
you can sign w ssh key w 2 git configs set. im not sure this qualifies as pain
- cryptonector 3y ago
  It's meaningless unless your keys are known to others to speak for you. I.e., you have to participate in the PGP trust mesh, and that is what is a pain.
  - AlwaysRock 3y ago
    Yup. This.
  - woodrowbarlow 3y ago
    isn't this what keybase is for?
  - arccy 3y ago
    or github.com/<username>.keys
  - Arnavion 3y ago
    https://datatracker.ietf.org/doc/draft-koch-openpgp-webkey-service/17/ https://datatracker.ietf.org/doc/draft-koch-openpgp-webkey-s... Serve the key at a certain URL from a webserver on your email domain. Clients that trust the domain can trust the key that it serves.
influx 3y ago
1Password makes it easy to do with ssh keys and biometrics. It’s actually pretty slick.

nightpool 3y ago

Why does Github use the same GPG key for all users? Feels like it makes these kind of confused deputy attacks endemic to the entire platform rather than being contained to a single user account.

gwern 3y ago

> Since the first author name is zero characters long, the regex skips that line, and the fake second author line is used instead. Git ignores extra author lines after the first, so Codespaces looks at the second author line but Git looks at the first. This means we can create GitHub-signed commits with any author name+email. Classic weird machine exploit of looking at two independent ad hoc parsers and finding inputs where they desynchronize and enter different states.

n4jm4 3y ago

Relying on regexes instead of thoroughly tested parser AST's encourages vulnerabilities. Are they really scraping logs for credentials instead of using an API?

layer8 3y ago

This looks like a classic case of a parser mismatch vulnerability [0]. [0] https://www.brainonfire.net/blog/2022/04/11/what-is-parser-mismatch/ https://www.brainonfire.net/blog/2022/04/11/what-is-parser-m...

jwilk 3y ago
Discussed on HN: https://news.ycombinator.com/item?id=38743029 https://news.ycombinator.com/item?id=38743029 (22 comments)

1oooqooq 3y ago

the fact they haven't published an analysis with every single case this was abused, even if zero, in the past is proof enough this was a feature to someone.