7 ms·

Best part is the fix: https://github.com/sudo-project/sudo/commit/b5d2010b6514ff45693509273bb07df3abb0bf0a https://github.com/sudo-project/sudo/commit/b5d2010b

Best part is the fix: https://github.com/sudo-project/sudo/commit/b5d2010b6514ff45693509273bb07df3abb0bf0a https://github.com/sudo-project/sudo/commit/b5d2010b6514ff45... Time to delete sudo.

akersten 7y ago
Every time I see a fiddly fix like this where we're changing a buffer counter to a postfix instead of a prefix increment, I always think "why are we doing this shit ourselves?" We should have standardized a C API to safely read/write buffers from files, networks, etc. decades ago. But here we are in 2020 manually moving pointers around - and what a surprise that we're finding "whoops, another off-by-one error, you know how it goes." No one should be writing new code to read/write bytes. This should be settled. It's like a carpenter building their own nail gun each time they want to build a house.
- deleted 7y ago
  [deleted]
- scintill76 7y ago
  > we're changing a buffer counter to a postfix instead of a prefix increment Maybe I'm missing something, but wasn't that a pointless change? The value of the expression was unused, so post- and pre- are equivalent.
  - sillysaurusx 7y ago
    Yes. This thread is blind-leading-the-blind. Threads against C almost always are. When I was a pentester, out of more than 50 projects I was on, I always found at least a medium-severity bug (a bug which could allow an attacker to pwn the app using nothing but a web browser). Guess how many of those codebases were written in C? Zero. The smugness is endemic to people outside of security. I was smug too. There are comments I wrote that I cringe at, looking back. Pentesting changes your perspective on things. C isn't going away. Yes, let's make it safer (by pentesting). No, it's not a mortal sin to write C. I'll find ways to exploit your webapp regardless of the language.
    - fulafel 7y ago
      Can I ask what kind of apps you typically worked on? A big part of why people make a big deal of C is that there are lots of code bases that have been under a lot of scrutiny from security minded code reviewers and pentesters and still keep on yielding more vulnerabilities. Ie it's hard to make C safe even when you really try and are a good programmer. In contrast IME most pentesters in the biz work on code bases that have seen much less scrutiny and are often in-house and/or "enterprise" apps whose code was written by people who are probably more domain experts than generally good programmers or security-aware developers...
    - jaas 7y ago
      This is a naive and dangerous perspective. You're basically saying that memory safe languages don't solve all problems so it's fine to use unsafe languages. Don't write code in unsafe languages unless you have to (almost never for new code). Period. You will have some problems either way, but you will have fewer problems in a memory safe language. Pen testing is not even remotely a good mitigation to depend on against the dangers of a language like C. Its fine to do for defense in depth and to find problems not related to memory safety, but it's no substitute for making a more sensible language choice.
      - loeg 7y ago
        Sudo isn't new code in any sense. "Rewrite the world in memory safe languages" is appealing if you completely discount the time and cost required to do so.
        jaas 7y ago
        I didn't say that.
        desc 7y ago
        And all the new bugs introduced in the process.
      - pjmlp 7y ago
        That is the classical defense from C camp, going back to the 80's Morris worm.
  - MaulingMonkey 7y ago
    It is a pointless change in that diff yeah. Perhaps it's meant to match a style guide that recommends preferring postfix operators? Or perhaps it's editing shrapnel from an earlier version of the changelist, that maybe used the result of that statement?
- codeflo 7y ago
  The prefix/postfix change is a red herring, that doesn't do anything (since the expression result isn't used). Which means that the security fix is interspersed with code style changes. Maybe there's a reason to do this here, but it's often not considered good practice because it makes it harder to review the code. AFAICT, the only real changes are in (new) line 411, resetting the cp variable after the loop, and line 416, ignoring write errors. The former is probably the relevant one.
  - MaulingMonkey 7y ago
    198 also disables password feedback mode in !tty mode. So, style changes, behavior changes, and security fixes all in one changelist. This is a small enough diff that I wouldn't complain if given this to review - but I probably also would've reverted the style changes (and would give myself at least 50/50 odds of splitting the changelist in two) Perhaps I've just been too broken by 1000+ LOC diffs that intersperse multiple behavior changes and style and refactoring and ...
  - stanferder 7y ago
    > The prefix/postfix change is a red herring, that doesn't do anything The prefix/postfix change does something to the reader. It's a distraction trying to figure out if that change was intended, whether the author knows it has no effect, or whether I'm wrong about the change having no effect. I appreciate the author doing the hard work of fixing this, especially with the code in full view of the public, but if I were an official reviewer I'd ask to get all the unnecessary changes removed.
  - nullc 7y ago
    Better than OpenSSL putting out critical security fixes in a 400kloc diff...
  - kohtatsu 7y ago
    PSA: git add -p and git commit -p exist and are beautiful.
    - jwilk 7y ago
- userbinator 7y ago
  If anything the solution is more low-level education, not the exact opposite. People who understand these essential basics, e.g. by starting with Asm, are going to be far less likely to make these sorts of mistakes and write less buggy code in general. I mean, a buffer overflow is a very simple concept and it's not hard to understand. How many bytes could be written, and how much storage is reserved? It's really just arithmetic. The amount of anti-education authoritarian fearmongering in these discussions is disturbing. Then again, given how much everything seems to be rapidly moving in the direction of dystopian corporatocracy, perhaps that's not so surprising. There's already a few comments about how "safer" languages don't really solve anything. They just push the problems up higher in the stack of abstractions... and when that time comes, what are the chances the "security" fearmongerers will just start blaming something else? It's fundamentally a problem of competence, and doing everything you can to make the world a prison in pursuit of that "perfect security" is really not a good idea. There's no replacement for intelligence.
  - pjmlp 7y ago
    Security reports from Google and Microsoft's teams prove otherwise. Also let's not forget that C exploits go back to the 80's, when most of us where doing full application in Assembly. Supposedly we knew our pointers pretty well.
    - shakna 7y ago
      > Also let's not forget that C exploits go back to the 80's, when most of us where doing full application in Assembly. I'm not sure this statement adds anything to your argument. "Exploits go back as far as the language was in common use" doesn't really mean anything. I'm sure Ada has had exploits, especially around its early lifetime. But its a language that's supposed to be safe.
      - AgentME 7y ago
        The existence or non-existence of exploits isn't very interesting. The rate that users make vulnerable code is more interesting. The fact that remote-code execution vulnerabilities are still very common in C codebases even after people and the ecosystem have had decades to learn the ins and outs of C is concerning. Maybe the rate of RCE occurrences in C codebases has gone down over time as people have been educated about vulnerabilities more, but I would guess that the rate of RCE vulnerabilities in C codebases is still much higher than the rate ever was for safer languages.
- fiddlerwoaroof 7y ago
  As far as I can tell, the prefix/postfix thing here is irrelevant and mostly done for style reasons? It’s on its own line and the value of the expression isn’t used, unless I’m missing something obvious.

Best part is the fix: https://github.com/sudo-project/sudo/commit/b5d2010b6514ff45693509273bb07df3abb0bf0a https://github.com/sudo-project/sudo/commit/b5d2010b6514ff45... Time to delete sudo.

akersten 7y ago

Every time I see a fiddly fix like this where we're changing a buffer counter to a postfix instead of a prefix increment, I always think "why are we doing this shit ourselves?" We should have standardized a C API to safely read/write buffers from files, networks, etc. decades ago. But here we are in 2020 manually moving pointers around - and what a surprise that we're finding "whoops, another off-by-one error, you know how it goes." No one should be writing new code to read/write bytes. This should be settled. It's like a carpenter building their own nail gun each time they want to build a house.

deleted 7y ago
[deleted]
scintill76 7y ago
> we're changing a buffer counter to a postfix instead of a prefix increment Maybe I'm missing something, but wasn't that a pointless change? The value of the expression was unused, so post- and pre- are equivalent.
- sillysaurusx 7y ago
  Yes. This thread is blind-leading-the-blind. Threads against C almost always are. When I was a pentester, out of more than 50 projects I was on, I always found at least a medium-severity bug (a bug which could allow an attacker to pwn the app using nothing but a web browser). Guess how many of those codebases were written in C? Zero. The smugness is endemic to people outside of security. I was smug too. There are comments I wrote that I cringe at, looking back. Pentesting changes your perspective on things. C isn't going away. Yes, let's make it safer (by pentesting). No, it's not a mortal sin to write C. I'll find ways to exploit your webapp regardless of the language.
  - fulafel 7y ago
    Can I ask what kind of apps you typically worked on? A big part of why people make a big deal of C is that there are lots of code bases that have been under a lot of scrutiny from security minded code reviewers and pentesters and still keep on yielding more vulnerabilities. Ie it's hard to make C safe even when you really try and are a good programmer. In contrast IME most pentesters in the biz work on code bases that have seen much less scrutiny and are often in-house and/or "enterprise" apps whose code was written by people who are probably more domain experts than generally good programmers or security-aware developers...
  - jaas 7y ago
    This is a naive and dangerous perspective. You're basically saying that memory safe languages don't solve all problems so it's fine to use unsafe languages. Don't write code in unsafe languages unless you have to (almost never for new code). Period. You will have some problems either way, but you will have fewer problems in a memory safe language. Pen testing is not even remotely a good mitigation to depend on against the dangers of a language like C. Its fine to do for defense in depth and to find problems not related to memory safety, but it's no substitute for making a more sensible language choice.
    - loeg 7y ago
      Sudo isn't new code in any sense. "Rewrite the world in memory safe languages" is appealing if you completely discount the time and cost required to do so.
      - jaas 7y ago
        I didn't say that.
      - desc 7y ago
        And all the new bugs introduced in the process.
    - pjmlp 7y ago
      That is the classical defense from C camp, going back to the 80's Morris worm.
- MaulingMonkey 7y ago
  It is a pointless change in that diff yeah. Perhaps it's meant to match a style guide that recommends preferring postfix operators? Or perhaps it's editing shrapnel from an earlier version of the changelist, that maybe used the result of that statement?
codeflo 7y ago
The prefix/postfix change is a red herring, that doesn't do anything (since the expression result isn't used). Which means that the security fix is interspersed with code style changes. Maybe there's a reason to do this here, but it's often not considered good practice because it makes it harder to review the code. AFAICT, the only real changes are in (new) line 411, resetting the cp variable after the loop, and line 416, ignoring write errors. The former is probably the relevant one.
- MaulingMonkey 7y ago
  198 also disables password feedback mode in !tty mode. So, style changes, behavior changes, and security fixes all in one changelist. This is a small enough diff that I wouldn't complain if given this to review - but I probably also would've reverted the style changes (and would give myself at least 50/50 odds of splitting the changelist in two) Perhaps I've just been too broken by 1000+ LOC diffs that intersperse multiple behavior changes and style and refactoring and ...
- stanferder 7y ago
  > The prefix/postfix change is a red herring, that doesn't do anything The prefix/postfix change does something to the reader. It's a distraction trying to figure out if that change was intended, whether the author knows it has no effect, or whether I'm wrong about the change having no effect. I appreciate the author doing the hard work of fixing this, especially with the code in full view of the public, but if I were an official reviewer I'd ask to get all the unnecessary changes removed.
- nullc 7y ago
  Better than OpenSSL putting out critical security fixes in a 400kloc diff...
- kohtatsu 7y ago
  PSA: git add -p and git commit -p exist and are beautiful.
  - jwilk 7y ago
userbinator 7y ago
If anything the solution is more low-level education, not the exact opposite. People who understand these essential basics, e.g. by starting with Asm, are going to be far less likely to make these sorts of mistakes and write less buggy code in general. I mean, a buffer overflow is a very simple concept and it's not hard to understand. How many bytes could be written, and how much storage is reserved? It's really just arithmetic. The amount of anti-education authoritarian fearmongering in these discussions is disturbing. Then again, given how much everything seems to be rapidly moving in the direction of dystopian corporatocracy, perhaps that's not so surprising. There's already a few comments about how "safer" languages don't really solve anything. They just push the problems up higher in the stack of abstractions... and when that time comes, what are the chances the "security" fearmongerers will just start blaming something else? It's fundamentally a problem of competence, and doing everything you can to make the world a prison in pursuit of that "perfect security" is really not a good idea. There's no replacement for intelligence.
- pjmlp 7y ago
  Security reports from Google and Microsoft's teams prove otherwise. Also let's not forget that C exploits go back to the 80's, when most of us where doing full application in Assembly. Supposedly we knew our pointers pretty well.
  - shakna 7y ago
    > Also let's not forget that C exploits go back to the 80's, when most of us where doing full application in Assembly. I'm not sure this statement adds anything to your argument. "Exploits go back as far as the language was in common use" doesn't really mean anything. I'm sure Ada has had exploits, especially around its early lifetime. But its a language that's supposed to be safe.
    - AgentME 7y ago
      The existence or non-existence of exploits isn't very interesting. The rate that users make vulnerable code is more interesting. The fact that remote-code execution vulnerabilities are still very common in C codebases even after people and the ecosystem have had decades to learn the ins and outs of C is concerning. Maybe the rate of RCE occurrences in C codebases has gone down over time as people have been educated about vulnerabilities more, but I would guess that the rate of RCE vulnerabilities in C codebases is still much higher than the rate ever was for safer languages.
fiddlerwoaroof 7y ago
As far as I can tell, the prefix/postfix thing here is irrelevant and mostly done for style reasons? It’s on its own line and the value of the expression isn’t used, unless I’m missing something obvious.