Hacker News Reader

Comment by YetAnotherNick | Hacker News Reader

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.

View on HN

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).

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.

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.

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.

Personally I’d have separated finding the Author line from pulling out the contents of that line - but still used Regex

When the data itself is just plain text, are there any other (better) ways?

Parsers?

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.

Regex is the parser for regular languages.

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?

We have libgit2, but github have their own ruby implementation of everything.

(java and rust have their own "native" implementation too.

libgit2 was written after github. So that might explain.

libgit2's frontpage claims that GitHub does use them: https://libgit2.org/

Depends which part of github uses them. Might even be their GUI client…

Are there alternatives that make it easier to think about all the edge cases?

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.

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.

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.

How did we get from criticizing regexes to mocking “rewrite in rust” in the span of three comments?

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.

> 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.

Hmm. Okay.

The part that bewilders me is that Rust showed up. Which would have made sense if someone was complaining about C++ or something, or maybe unsafe memory management. But regexes?

> Either way, I didn’t want to have a discussion about rust.

Which is why you brought it up unprompted... I give up.

Okay, I’ll give you one more thing: I love using rust. I also use zoxide, ripgrep, dua, etc.

I don’t hate the language. Quite the opposite. I hope you can now go back and just see the argument for what it is and not for what you thought I was insinuating.

Hey, watch what you’re saying buddy, ‘rewrite in rust’ is what pays the bills in this family!

Reply on news.ycombinator.com