4 ms·

Money quote from the message thread: "I can't even find the words to express how gross this is." (http://www.openldap.org/lists/openldap-devel/200802/msg00100.h

Money quote from the message thread: "I can't even find the words to express how gross this is." (http://www.openldap.org/lists/openldap-devel/200802/msg00100.html http://www.openldap.org/lists/openldap-devel/200802/msg00100...) Interestingly, the conversation stays somewhat civil even after that quote, looks like professionals at work :)

grannyg00se 13y ago
Probably helps that the message containing that quote also contained a handful of significant examples of poor coding choices along with suggestions for improvement.
ajross 13y ago
The quote seems to simply be complaining about the repeated use of the subexpression "strlen(str)", by implication because it's needless and inefficient. Except that it's not. At least on glibc, strlen() is declared "pure" to the compiler and (unless otherwise defeated by pointer aliasing) repeated calls will be optimized away. That's not to say that this is the best way to write the code, but the concern seems poorly informed.
- steveklabnik 13y ago
  It's not an efficiency concern, it's a security concern.
  - ajross 13y ago
    Common subexpression elimination isn't normally considered a security technique, so I guess I don't understand your point. Maybe you're saying it's a "code smell" kind of thing and that being sloppy here indicates more subtle problems elsewhere? Which then hits the argument about whether this is really "sloppy" or just intentionally simple. Shrug. My point was just that this needs better evidence. There is no demonstrated bug in the linked code, and the assertion that it is "gross" (OP) or "insecure" (you) seems poorly justified.
    - Xylakant 13y ago
      The basic concern is that strlen() shouldn't be used at all on the data that's passed to the given function since that data may be binary and not - as the function assumes - null-terminated string. The code is "sloppy" and the complete certificate handling seems to be sloppy. I don't want to be the judge of that, but if you read the whole post and the ensuing thread, the argument is made quite well and convincingly. And seriously, the last place I'd like to see a sloppy implementation that assumes that the given data is benevolent and does not contain a malicious payload is - guess what - a TLS library.
      - ajross 13y ago
        Yes yes yes, but the specific "money quote" above wasn't talking about that, it was talking about the number of redundant calls to strlen() in this one particular function. Extending it to mean something else by our implication makes it something rather different than a "money quote".
      - ambrice 13y ago
        There's only so much an API can do with garbage input. If this function took a pointer and a length, that's not a magic fix, you could still pass it a bad pointer and/or a bad length..
        Xylakant 13y ago
        The problem here is that binary input is valid according to the spec [1]. It's not malicious input in the sense that a programmer is using an interface deliberately wrong but rather in the sense that a counterparty could send you a non-garbage certificate that contains that data - which would be valid, but still break this code. That's not comparable to passing a bad pointer or a bad length. [1] at least according to the post. The fact that gnutls added a binary interface later seems to support that reading.
        ambrice 13y ago
        Yes, the spec says the field can be string or binary. The API only handles string fields. The API should be (and was) updated with a function that handles binary fields, but there's nothing wrong with the original function. If this was C++, and the function took a std::string, would you say it was horribly broken because you serialized a 4 byte IP address into a 4 byte std::string buffer and the function didn't handle it correctly?
- nly 13y ago
  > (unless otherwise defeated by pointer aliasing) In practice calling any other non-pure function, which the compiler can't see in to, will defeat this optimisation, whether there's aliasing within your function or not.
defen 13y ago
Any time I see code like: if ((len_len + (int) strlen (str)) <= max_len) with int's I immediately start worrying about integer overflow leading to buffer overflow. Nobody seems to have mentioned this though.

grannyg00se 13y ago

Probably helps that the message containing that quote also contained a handful of significant examples of poor coding choices along with suggestions for improvement.

ajross 13y ago

The quote seems to simply be complaining about the repeated use of the subexpression "strlen(str)", by implication because it's needless and inefficient. Except that it's not. At least on glibc, strlen() is declared "pure" to the compiler and (unless otherwise defeated by pointer aliasing) repeated calls will be optimized away. That's not to say that this is the best way to write the code, but the concern seems poorly informed.

steveklabnik 13y ago
It's not an efficiency concern, it's a security concern.
- ajross 13y ago
  Common subexpression elimination isn't normally considered a security technique, so I guess I don't understand your point. Maybe you're saying it's a "code smell" kind of thing and that being sloppy here indicates more subtle problems elsewhere? Which then hits the argument about whether this is really "sloppy" or just intentionally simple. Shrug. My point was just that this needs better evidence. There is no demonstrated bug in the linked code, and the assertion that it is "gross" (OP) or "insecure" (you) seems poorly justified.
  - Xylakant 13y ago
    The basic concern is that strlen() shouldn't be used at all on the data that's passed to the given function since that data may be binary and not - as the function assumes - null-terminated string. The code is "sloppy" and the complete certificate handling seems to be sloppy. I don't want to be the judge of that, but if you read the whole post and the ensuing thread, the argument is made quite well and convincingly. And seriously, the last place I'd like to see a sloppy implementation that assumes that the given data is benevolent and does not contain a malicious payload is - guess what - a TLS library.
    - ajross 13y ago
      Yes yes yes, but the specific "money quote" above wasn't talking about that, it was talking about the number of redundant calls to strlen() in this one particular function. Extending it to mean something else by our implication makes it something rather different than a "money quote".
    - ambrice 13y ago
      There's only so much an API can do with garbage input. If this function took a pointer and a length, that's not a magic fix, you could still pass it a bad pointer and/or a bad length..
      - Xylakant 13y ago
        The problem here is that binary input is valid according to the spec [1]. It's not malicious input in the sense that a programmer is using an interface deliberately wrong but rather in the sense that a counterparty could send you a non-garbage certificate that contains that data - which would be valid, but still break this code. That's not comparable to passing a bad pointer or a bad length. [1] at least according to the post. The fact that gnutls added a binary interface later seems to support that reading.
        ambrice 13y ago
        Yes, the spec says the field can be string or binary. The API only handles string fields. The API should be (and was) updated with a function that handles binary fields, but there's nothing wrong with the original function. If this was C++, and the function took a std::string, would you say it was horribly broken because you serialized a 4 byte IP address into a 4 byte std::string buffer and the function didn't handle it correctly?
nly 13y ago
> (unless otherwise defeated by pointer aliasing) In practice calling any other non-pure function, which the compiler can't see in to, will defeat this optimisation, whether there's aliasing within your function or not.

defen 13y ago

Any time I see code like: if ((len_len + (int) strlen (str)) <= max_len) with int's I immediately start worrying about integer overflow leading to buffer overflow. Nobody seems to have mentioned this though.