5 ms·

https://www.theregister.co.uk/2017/11/29/apple_macos_high_sierra_root_bug_patch/ https://www.theregister.co.uk/2017/11/29/apple_macos_high_si... The relevant f

by runeks 9y ago

https://www.theregister.co.uk/2017/11/29/apple_macos_high_sierra_root_bug_patch/ https://www.theregister.co.uk/2017/11/29/apple_macos_high_si... The relevant function returns an opaque integer to signal failure/success. If this were typed properly — e.g. a type that represents either “Success” or “Failure” explicitly (rather than implicitly via an int) — the bug would be unlikely to happen, since it would require the function in question to explicitly return “Success” when in fact it had failed (as opposed to the opaque 0x01).

btown 9y ago
In this specific case, strongly typing the CryptVerificationResult as an enum would have helped. But, generalizing the problem, if you had more complicated criteria for whether that branch should be taken, it can be far too easy to save an intermediate result into a (strongly typed) variable but never end up using it in the correct way in the condition statement itself. Rust goes part of the way here by ensuring that Result variables get checked: https://doc.rust-lang.org/std/result/#results-must-be-used https://doc.rust-lang.org/std/result/#results-must-be-used which to my knowledge is not something you can mimic in C/C++. But you could still forget a negation or use && instead of || somewhere, and have an uncommon code path fail. Code review reduces but doesn't eliminate the probability of these complex logic errors. What's really needed is tooling that ensures test coverage on a phrase-by-phrase, not line-by-line, basis. (Basic line coverage would have said that all these lines of code were executed in testing.) And you need a culture around paying attention to those results. That can be very difficult to build, but for mission-critical software (security included) you absolutely need that level of attention to detail.
- steveklabnik 9y ago
  gcc has __attribute__((warn_unused_result)) which is similar to #[must_use].
- KirinDave 9y ago
  > In this specific case, strongly typing the CryptVerificationResult as an enum would have helped. But, generalizing the problem, if you had more complicated criteria for whether that branch should be taken, it can be far too easy to save an intermediate result into a (strongly typed) variable but never end up using it in the correct way in the condition statement itself. We can capture that requirement with something called Linear typing, but even if we don't go for a compiler-enforced consumption the creation of a universally used family of result enums brings enormous discipline and reliability to error handling. In no small part because when considering the result of such an enum, the compiler can demand a total pattern match, which forces developers to consider what that failure means in context and present SOME kind of strategy (even if it's hard failure). The approaches of languages with type inference and more sophisticated type systems like OCaml and Haskell go even a step further because they can create workflows around these types, creating composite workflows that make it easy to handle errors. It becomes harder to ignore them, or write functions that ignore them. One of the reasons it's so easy to write parsers in languages like Haskell is that algebraic data types and applicative functor composition make it easy and even convenient to talk about the logic within the context of non-trivial error flows. It's much more frustrating to write a parser without a combinator framework in OCaml or Haskell. Similar stories exist for Validator, Either, and Maybe/Option. These techniques don't mandate error processing, but they make it more convenient to compose error-handling functions and offer more compiler checking when the results must be handled.
z3t4 9y ago
While disassembled code is hard to read I think it's not a problem with weak types. The problem seems to be the root account got enabled without a password.
- KirinDave 9y ago
  It is crystal-clear a type error, but here's an article breaking it down: https://objective-see.com/blog/blog_0x24.html https://objective-see.com/blog/blog_0x24.html notes that od_verify_crypt_password is called with an error check expecting 0, but on failure it returns 0x1. The code then goes on to a "success" path with an "update". This is a classical type mismatch that C and C++ pre-2012 are so famous for. Someone writing this program where both modules were in Rust would have used Option or Result and this expectation mismatch wouldn't have existed.
  - z3t4 9y ago
    It is still not clear, it seems more like a logic error to me. Zero means success and a non zero value is an error code. It basically says "if login fails, then do this". If they would have used a boolean instead, or a result enum, the code would have taken the same path! I argue that strong typing would not have detected the logic error! The bad code was probably rushed in to make the upgrade work. All tests turning up green can really put you into a false sense of security. But that is another strongly opinioned topic!
    - KirinDave 9y ago
      You've just defined two type errors.

by runeks 9y ago

btown 9y ago

In this specific case, strongly typing the CryptVerificationResult as an enum would have helped. But, generalizing the problem, if you had more complicated criteria for whether that branch should be taken, it can be far too easy to save an intermediate result into a (strongly typed) variable but never end up using it in the correct way in the condition statement itself. Rust goes part of the way here by ensuring that Result variables get checked: https://doc.rust-lang.org/std/result/#results-must-be-used https://doc.rust-lang.org/std/result/#results-must-be-used which to my knowledge is not something you can mimic in C/C++. But you could still forget a negation or use && instead of || somewhere, and have an uncommon code path fail. Code review reduces but doesn't eliminate the probability of these complex logic errors. What's really needed is tooling that ensures test coverage on a phrase-by-phrase, not line-by-line, basis. (Basic line coverage would have said that all these lines of code were executed in testing.) And you need a culture around paying attention to those results. That can be very difficult to build, but for mission-critical software (security included) you absolutely need that level of attention to detail.

steveklabnik 9y ago
gcc has __attribute__((warn_unused_result)) which is similar to #[must_use].
KirinDave 9y ago
> In this specific case, strongly typing the CryptVerificationResult as an enum would have helped. But, generalizing the problem, if you had more complicated criteria for whether that branch should be taken, it can be far too easy to save an intermediate result into a (strongly typed) variable but never end up using it in the correct way in the condition statement itself. We can capture that requirement with something called Linear typing, but even if we don't go for a compiler-enforced consumption the creation of a universally used family of result enums brings enormous discipline and reliability to error handling. In no small part because when considering the result of such an enum, the compiler can demand a total pattern match, which forces developers to consider what that failure means in context and present SOME kind of strategy (even if it's hard failure). The approaches of languages with type inference and more sophisticated type systems like OCaml and Haskell go even a step further because they can create workflows around these types, creating composite workflows that make it easy to handle errors. It becomes harder to ignore them, or write functions that ignore them. One of the reasons it's so easy to write parsers in languages like Haskell is that algebraic data types and applicative functor composition make it easy and even convenient to talk about the logic within the context of non-trivial error flows. It's much more frustrating to write a parser without a combinator framework in OCaml or Haskell. Similar stories exist for Validator, Either, and Maybe/Option. These techniques don't mandate error processing, but they make it more convenient to compose error-handling functions and offer more compiler checking when the results must be handled.

z3t4 9y ago

While disassembled code is hard to read I think it's not a problem with weak types. The problem seems to be the root account got enabled without a password.

KirinDave 9y ago
It is crystal-clear a type error, but here's an article breaking it down: https://objective-see.com/blog/blog_0x24.html https://objective-see.com/blog/blog_0x24.html notes that od_verify_crypt_password is called with an error check expecting 0, but on failure it returns 0x1. The code then goes on to a "success" path with an "update". This is a classical type mismatch that C and C++ pre-2012 are so famous for. Someone writing this program where both modules were in Rust would have used Option or Result and this expectation mismatch wouldn't have existed.
- z3t4 9y ago
  It is still not clear, it seems more like a logic error to me. Zero means success and a non zero value is an error code. It basically says "if login fails, then do this". If they would have used a boolean instead, or a result enum, the code would have taken the same path! I argue that strong typing would not have detected the logic error! The bad code was probably rushed in to make the upgrade work. All tests turning up green can really put you into a false sense of security. But that is another strongly opinioned topic!
  - KirinDave 9y ago
    You've just defined two type errors.