9 ms·

Diff of 3.0.6 and 3.0.7: https://gist.github.com/FiloSottile/611fc3fa95c3aceebf2580983f76148c https://gist.github.com/FiloSottile/611fc3fa95c3aceebf258098... v

by miduil 4y ago

Diff of 3.0.6 and 3.0.7: https://gist.github.com/FiloSottile/611fc3fa95c3aceebf2580983f76148c https://gist.github.com/FiloSottile/611fc3fa95c3aceebf258098... via https://www.twitch.tv/filosottile https://www.twitch.tv/filosottile

formerly_proven 4y ago
Specifically https://gist.github.com/FiloSottile/611fc3fa95c3aceebf2580983f76148c#file-openssl-3-0-6-to-3-0-7-diff-L493 https://gist.github.com/FiloSottile/611fc3fa95c3aceebf258098... - if (written_out > max_out) + if (written_out >= max_out) return 0; Plus a second, less memeable fix.
- jeffbee 4y ago
  - hedora 4y ago
    I don't think it is fair to blame the language for this. Look at the lines above the vulnerability: n = n + i / (written_out + 1); i %= (written_out + 1); Whatever buffer manipulation this is doing could be abstracted out into a library. (As studio.h does.) Also, since written_out probably varies from loop iteration to iteration, what does "n remainder i" even mean? Also, why don't they increment written_out before doing the arithmetic, and why do they increment i on line 523, when the only intermediate use of it is "i + 1"? I've written some crappy code in my time, but when things descended into that level of insanity, I either delegated to fuzz tested functions, or at least left a comment.
  - Hackbraten 4y ago
    You’re not wrong. Back in the day, you didn’t have alternatives with zero-cost abstractions like we have today. There are many valid reasons why one would make decisions to balance performance against memory-safety, especially 25 years ago – and given the fact that networking code tends to be part of hot paths where optimization matters.
  - sirtomato 4y ago
    come on, that is quite harsh. c is easily my favorite programming language and it just works really damn well and produces very small binaries. it's easy to write terrible code in any programming language.
    - School-Cotton 4y ago
      > c is easily my favorite programming language That's not really relevant to the discussion of whether it's the proper language to be used for something like OpenSSL. > it just works really damn well As far as I can tell, all mainstream programming languages "work well", so it's not clear what you mean. > produces very small binaries True but not very important IMO > it's easy to write terrible code in any programming language Sure. But it's not easy to write this particular flavor of terrible code -- buffer overflows -- in any commonly-used language other than C and its derivatives.
    - jeffbee 4y ago
      C++ generally produces smaller and faster programs because the language gives compilers much more to work with.
    - deleted 4y ago
      [deleted]
  - Sohcahtoa82 4y ago
    To be fair, OpenSSL was first released in 1998. We didn't have fast memory-safe languages back then. Java existed, but was incredibly slow.
    - jeffbee 4y ago
      The function in question was written in 2020. OpenSSL 3 is an incompatible major rewrite undertaken during 2018-2021 and there is no no valid excuse for why language quality was not improved at that time.
      - Sohcahtoa82 4y ago
        > OpenSSL 3 is an incompatible major rewrite Oh snap I didn't know that, and I probably should have. TIL. Then yeah, you're right. It probably should have been written in Rust.
- jancsika 4y ago
  I don't understand this one: - (written_out - i) * sizeof *pDecoded); + (written_out - i) * sizeof(*pDecoded)); Edit: just to be clear-- I'm claiming there's no way the addition of parentheses could change the behavior of the program. I guess I can understand resolving an ambiguity so that the reader doesn't have to look up precedence rules. But when combined with a high-profile bug in a critical piece of infrastructure it looks a bit like the software equivalent of not shaving one's beard in the hopes of winning a sports tournament.
  - bewaretheirs 4y ago
    Looks to me like a coding style fix along for the ride with the correctness fix.
  - Retr0id 4y ago
    I don't think that's security-relevant, likely just keeping a linter happy.
    - jancsika 4y ago
      Ah, I didn't think about a linter. I'll take a look at the code later to see if that's the case.
  - nurettin 4y ago
    sizeof is a keyword because of some terrible reason, but I'd parenthesize it just to make the intention clear.
    - Sohcahtoa82 4y ago
      Agreed. sizeof FEELS like a function, and so I use parentheses with it, even though I know it's not actually a function.
      - krylon 4y ago
        Wasn't there some subtle difference between using sizeof with vs without parens? Something like if you pass a type to sizeof -- e.g. sizeof(*int) -- you must use parents anyway? It's been years since I've used C, I'm probably not remembering it correctly.
- hedora 4y ago
  From the variable names, it is clear that the old version was correct. Someone should revert this. :-)
deleted 4y ago
[deleted]

by miduil 4y ago

formerly_proven 4y ago

Specifically https://gist.github.com/FiloSottile/611fc3fa95c3aceebf2580983f76148c#file-openssl-3-0-6-to-3-0-7-diff-L493 https://gist.github.com/FiloSottile/611fc3fa95c3aceebf258098... - if (written_out > max_out) + if (written_out >= max_out) return 0; Plus a second, less memeable fix.

jeffbee 4y ago
- hedora 4y ago
  I don't think it is fair to blame the language for this. Look at the lines above the vulnerability: n = n + i / (written_out + 1); i %= (written_out + 1); Whatever buffer manipulation this is doing could be abstracted out into a library. (As studio.h does.) Also, since written_out probably varies from loop iteration to iteration, what does "n remainder i" even mean? Also, why don't they increment written_out before doing the arithmetic, and why do they increment i on line 523, when the only intermediate use of it is "i + 1"? I've written some crappy code in my time, but when things descended into that level of insanity, I either delegated to fuzz tested functions, or at least left a comment.
- Hackbraten 4y ago
  You’re not wrong. Back in the day, you didn’t have alternatives with zero-cost abstractions like we have today. There are many valid reasons why one would make decisions to balance performance against memory-safety, especially 25 years ago – and given the fact that networking code tends to be part of hot paths where optimization matters.
- sirtomato 4y ago
  come on, that is quite harsh. c is easily my favorite programming language and it just works really damn well and produces very small binaries. it's easy to write terrible code in any programming language.
  - School-Cotton 4y ago
    > c is easily my favorite programming language That's not really relevant to the discussion of whether it's the proper language to be used for something like OpenSSL. > it just works really damn well As far as I can tell, all mainstream programming languages "work well", so it's not clear what you mean. > produces very small binaries True but not very important IMO > it's easy to write terrible code in any programming language Sure. But it's not easy to write this particular flavor of terrible code -- buffer overflows -- in any commonly-used language other than C and its derivatives.
  - jeffbee 4y ago
    C++ generally produces smaller and faster programs because the language gives compilers much more to work with.
  - deleted 4y ago
    [deleted]
- Sohcahtoa82 4y ago
  To be fair, OpenSSL was first released in 1998. We didn't have fast memory-safe languages back then. Java existed, but was incredibly slow.
  - jeffbee 4y ago
    The function in question was written in 2020. OpenSSL 3 is an incompatible major rewrite undertaken during 2018-2021 and there is no no valid excuse for why language quality was not improved at that time.
    - Sohcahtoa82 4y ago
      > OpenSSL 3 is an incompatible major rewrite Oh snap I didn't know that, and I probably should have. TIL. Then yeah, you're right. It probably should have been written in Rust.
jancsika 4y ago
I don't understand this one: - (written_out - i) * sizeof *pDecoded); + (written_out - i) * sizeof(*pDecoded)); Edit: just to be clear-- I'm claiming there's no way the addition of parentheses could change the behavior of the program. I guess I can understand resolving an ambiguity so that the reader doesn't have to look up precedence rules. But when combined with a high-profile bug in a critical piece of infrastructure it looks a bit like the software equivalent of not shaving one's beard in the hopes of winning a sports tournament.
- bewaretheirs 4y ago
  Looks to me like a coding style fix along for the ride with the correctness fix.
- Retr0id 4y ago
  I don't think that's security-relevant, likely just keeping a linter happy.
  - jancsika 4y ago
    Ah, I didn't think about a linter. I'll take a look at the code later to see if that's the case.
- nurettin 4y ago
  sizeof is a keyword because of some terrible reason, but I'd parenthesize it just to make the intention clear.
  - Sohcahtoa82 4y ago
    Agreed. sizeof FEELS like a function, and so I use parentheses with it, even though I know it's not actually a function.
    - krylon 4y ago
      Wasn't there some subtle difference between using sizeof with vs without parens? Something like if you pass a type to sizeof -- e.g. sizeof(*int) -- you must use parents anyway? It's been years since I've used C, I'm probably not remembering it correctly.
hedora 4y ago
From the variable names, it is clear that the old version was correct. Someone should revert this. :-)

deleted 4y ago

[deleted]