4 ms·

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

by 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?
  - nradov 3y ago
    You're not wrong, but sometimes you have to be pragmatic. I've done a lot of work building interfaces to acquire clinical data from healthcare provider organizations for care quality improvement. There are clear industry standards from HL7 (V2 Messaging, CDA, FHIR) but many provider organizations don't implement them correctly, so we had to accept a lot of junk and just find a way to deal with it. Many of those provider organizations lack IT resources and are at the mercy of their vendors or consultants, so trying to hold them to strict standards results in an impasse.
  - treflop 3y ago
    Idk it makes sense for UX sometimes. Really it’s a case by case basis and I don’t think you should be adhering to any law for this.
a1o 3y ago
> In computing, the robustness principle is a design guideline for software that states: "be conservative in what you do, be liberal in what you accept from others". It is often reworded as: "be conservative in what you send, be liberal in what you accept". The principle is also known as Postel's law, after Jon Postel, who used the wording in an early specification of TCP. I didn't know, so if anyone wants to know, here's the summary from Wikipedia. https://en.wikipedia.org/wiki/Robustness_principle https://en.wikipedia.org/wiki/Robustness_principle
- jrockway 3y ago
  It's a tough balance. Postel's law is from a world before security mattered. It is incompatible with correctness. The early Internet definitely wouldn't have worked without a thick layer of "you set this flag wrong in the header but I know what you meant". But these liberal formats have been the death of many a "0 remote root holes in the default install" OS slogan ;) Git, meanwhile, would be something like: $ git clone example.com/cool-code FATAL: commit abc123: 2 errors: header.author: too many authors in header header.author[0]: empty friendly name checkout aborted, contact the upstream git repo and tell 'em it's broke People would be mad and would stop using Git, so "git clone" has to accept whatever. But the signature generating-code doesn't need to be this liberal; it can run this validation and say "I'm not going to sign that", and nobody would be mad. Github's implementation is just a shortcut; some engineer tried their flow against a test commit they made, it worked, they checked it in, they shipped the feature. Then someone thought "what if there are two author lines", and broke the signing code. (Maybe write some fuzz tests that emit headers and try signing them? You might not find this bug, but it can help.) The problem is the missing spec, but specs are useless if not enforced, because people won't know that they accidentally violated the spec. That's what's tough. Postel's Law guarantees working software. But sometimes working means failing open. (Failing closed can definitely suck. Ever been late to work because of a "signal problem"? The signalling system failed closed and prevented your train from moving even though there wasn't any actual reason to prevent movement. Postel's Law would say "it's just a burned out light bulb; power ahead at line speed, not expecting a train randomly misrouted and barreling into you head on". 99.9% of the time, it's right! But there are a lot of dead 0.1%-ers, which is after the early days, we decided to make the system fail closed.)
surfingdino 3y ago
It makes sense when you want rapid adoption, as was the case with the Internet.
Arnavion 3y ago
This still has the same problem. Your solution requires them to program the first "Extract all author lines" step to accept both lines, namely they would have had to make sure their extractor considered author lines with a zero-length author name to be "an author line". But if they had the foresight to do that they could just as well have done it with their original regex to begin with.
- theamk 3y ago
  I don't see how? As I said, actually parsing the line is the 2nd step. The first step is to extract all lines which start from "author ", without checking what the rest of the line looks like. That could be something like starts_with?("author "); or split by first space and check if first word is "author"; or verify against "^author " regex; or take first 7 chars and compare against "author ". All of those methods would easily catch the duplicate line in the attack and reject it. (you might be curious if simple "author", without any argument, would be bad. First, even if it was, the impact would be pretty low as there is no second name to inject. Second, there is no need to worry: git actually checks for space after "author" [0], simple "author\n" by itself would be rejected) [0] https://github.com/git/git/blob/e02ecfcc534e2021aae29077a958dd11c3897e4c/commit.c#L1761 https://github.com/git/git/blob/e02ecfcc534e2021aae29077a958...
  - Arnavion 3y ago
    >The first step is to extract all lines which start from "author ", without checking what the rest of the line looks like. I'm saying that this already requires knowing that "without checking what the rest of the line looks like" is an important consideration. It would be just as valid to define "extract all author lines" as "extract all lines that have `author <value>`, as they did. Only by knowing that the latter definition causes the bug that the TFA found (either by thinking about it or by testing against the canonical git implementation) would it become known that that definition is wrong. The problem is not the structure of the parser. The problem is that they didn't check that their implementation matches the behavior of the system it speaks to (git). Edit: Also covered in https://news.ycombinator.com/item?id=39101419 https://news.ycombinator.com/item?id=39101419 / https://news.ycombinator.com/item?id=39101736 https://news.ycombinator.com/item?id=39101736 / https://news.ycombinator.com/item?id=39107945 https://news.ycombinator.com/item?id=39107945

by theamk 3y ago

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?
- nradov 3y ago
  You're not wrong, but sometimes you have to be pragmatic. I've done a lot of work building interfaces to acquire clinical data from healthcare provider organizations for care quality improvement. There are clear industry standards from HL7 (V2 Messaging, CDA, FHIR) but many provider organizations don't implement them correctly, so we had to accept a lot of junk and just find a way to deal with it. Many of those provider organizations lack IT resources and are at the mercy of their vendors or consultants, so trying to hold them to strict standards results in an impasse.
- treflop 3y ago
  Idk it makes sense for UX sometimes. Really it’s a case by case basis and I don’t think you should be adhering to any law for this.

a1o 3y ago

> In computing, the robustness principle is a design guideline for software that states: "be conservative in what you do, be liberal in what you accept from others". It is often reworded as: "be conservative in what you send, be liberal in what you accept". The principle is also known as Postel's law, after Jon Postel, who used the wording in an early specification of TCP. I didn't know, so if anyone wants to know, here's the summary from Wikipedia. https://en.wikipedia.org/wiki/Robustness_principle https://en.wikipedia.org/wiki/Robustness_principle

jrockway 3y ago
It's a tough balance. Postel's law is from a world before security mattered. It is incompatible with correctness. The early Internet definitely wouldn't have worked without a thick layer of "you set this flag wrong in the header but I know what you meant". But these liberal formats have been the death of many a "0 remote root holes in the default install" OS slogan ;) Git, meanwhile, would be something like: $ git clone example.com/cool-code FATAL: commit abc123: 2 errors: header.author: too many authors in header header.author[0]: empty friendly name checkout aborted, contact the upstream git repo and tell 'em it's broke People would be mad and would stop using Git, so "git clone" has to accept whatever. But the signature generating-code doesn't need to be this liberal; it can run this validation and say "I'm not going to sign that", and nobody would be mad. Github's implementation is just a shortcut; some engineer tried their flow against a test commit they made, it worked, they checked it in, they shipped the feature. Then someone thought "what if there are two author lines", and broke the signing code. (Maybe write some fuzz tests that emit headers and try signing them? You might not find this bug, but it can help.) The problem is the missing spec, but specs are useless if not enforced, because people won't know that they accidentally violated the spec. That's what's tough. Postel's Law guarantees working software. But sometimes working means failing open. (Failing closed can definitely suck. Ever been late to work because of a "signal problem"? The signalling system failed closed and prevented your train from moving even though there wasn't any actual reason to prevent movement. Postel's Law would say "it's just a burned out light bulb; power ahead at line speed, not expecting a train randomly misrouted and barreling into you head on". 99.9% of the time, it's right! But there are a lot of dead 0.1%-ers, which is after the early days, we decided to make the system fail closed.)

surfingdino 3y ago

It makes sense when you want rapid adoption, as was the case with the Internet.

Arnavion 3y ago

This still has the same problem. Your solution requires them to program the first "Extract all author lines" step to accept both lines, namely they would have had to make sure their extractor considered author lines with a zero-length author name to be "an author line". But if they had the foresight to do that they could just as well have done it with their original regex to begin with.

theamk 3y ago
I don't see how? As I said, actually parsing the line is the 2nd step. The first step is to extract all lines which start from "author ", without checking what the rest of the line looks like. That could be something like starts_with?("author "); or split by first space and check if first word is "author"; or verify against "^author " regex; or take first 7 chars and compare against "author ". All of those methods would easily catch the duplicate line in the attack and reject it. (you might be curious if simple "author", without any argument, would be bad. First, even if it was, the impact would be pretty low as there is no second name to inject. Second, there is no need to worry: git actually checks for space after "author" [0], simple "author\n" by itself would be rejected) [0] https://github.com/git/git/blob/e02ecfcc534e2021aae29077a958dd11c3897e4c/commit.c#L1761 https://github.com/git/git/blob/e02ecfcc534e2021aae29077a958...
- Arnavion 3y ago
  >The first step is to extract all lines which start from "author ", without checking what the rest of the line looks like. I'm saying that this already requires knowing that "without checking what the rest of the line looks like" is an important consideration. It would be just as valid to define "extract all author lines" as "extract all lines that have `author <value>`, as they did. Only by knowing that the latter definition causes the bug that the TFA found (either by thinking about it or by testing against the canonical git implementation) would it become known that that definition is wrong. The problem is not the structure of the parser. The problem is that they didn't check that their implementation matches the behavior of the system it speaks to (git). Edit: Also covered in https://news.ycombinator.com/item?id=39101419 https://news.ycombinator.com/item?id=39101419 / https://news.ycombinator.com/item?id=39101736 https://news.ycombinator.com/item?id=39101736 / https://news.ycombinator.com/item?id=39107945 https://news.ycombinator.com/item?id=39107945