7 ms·
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 ha
by 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 agoWhat 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 agoWe have libgit2, but github have their own ruby implementation of everything. (java and rust have their own "native" implementation too.
- Zababa 3y agoAre there alternatives that make it easier to think about all the edge cases?
- michaelt 3y agoKeep 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 agoThe 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 agoI 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 agoHow did we get from criticizing regexes to mocking “rewrite in rust” in the span of three comments?
- ylk 3y agoI’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 agoDisagree 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 agoUnless 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 agoWhen the data itself is just plain text, are there any other (better) ways?
- brokensegue 3y agoParsers?
- cedilla 3y agoThis 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 agoThis 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 agoPersonally I’d have separated finding the Author line from pulling out the contents of that line - but still used Regex