Hacker News Reader

Comment by Xylakant | Hacker News Reader

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...) Interestingly, the conversation stays somewhat civil even after that quote, looks like professionals at work :)

View on HN

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.

It's not an efficiency concern, it's a security concern.

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.

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.

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".

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

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.

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?

Seriously, if the function deals with untrusted user input and pretends to conform to a spec, weasel wording around by saying that the function does only partially conform to the spec and will blow up in the users face when passed other, spec compliant input and then deferring all responsibility to the user, yes, that counts as horribly broken in my books (even though you were the one to introduce those words). That's what we have libraries for, so that me and you don't have to deal with this mess that x509 cert parsing is - I've seen enough of it that I know I don't want to go down this particular hell hole - and I've just been standing at the sideline and watching others wrestle with it.

functions don't conform to specs, the API and library as a whole should conform to the spec. It's perfectly valid to have one function that supports strings and another function that supports binary data.

Did they have such a pair of functions?

They do now, and the first one is still using strlen(). Does the existence of the second function mean it's now ok for the first to use strlen()? You can still crash the program if you send binary data to the string function instead of the binary function..

So essentially it's an issue with C being weakly typed?

> (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.

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.

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

Reply on news.ycombinator.com