Make string_util::WriteInto() DCHECK() that the supplied |length_with_null| > 1, meaning that the...

ake string_util::WriteInto() DCHECK() that the supplied |length_with_null| > 1, meaning that the without-'\0' string is non-empty.  This replaces the conditional code added recently that makes this case return NULL.  It's easier to understand if it's simply an error to call WriteInto() in this case at all.

Add DCHECK()s or conditionals as appropriate to callers in order to ensure this assertion holds.

BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112005

[joth, 2011-11-01 11:25:32]

A couple drive-by comments that occurred to me while reading the openssl
changes.

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h
File base/string_util.h (left):

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h#oldcode478
base/string_util.h:478: return NULL;
Worth noting there are a couple drawbacks in removing this:
- removes consistency with string_as_array (note that used to have non portable
optimizations for this case too, but I removed them as we care more for
portability than the origin  of that did)
- in the case someone does access the return value it turns a 'simple' NULL
pointer deref into a platform-dependent undefined behavior. (Of course, we hope
the DCHECK does catch it before that goes into a release)

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h#newcode468
base/string_util.h:468: size_t length_with_null) {
completely orthogonal issue to the one you're tackling here, but do the majority
of callers have to explicitly +1 to there argument? I'd find this API a lot more
intuitive if the size I pass in was the same as the size of the string after the
call, and hence the same as the number of bytes I am then permitted to write
into it.

The + 1 for null is really just an internal optimization, to remove the risk a
subsequent call to c_str() has to incur a reallocation. It doesn't seem to have
any bearing on the observable contract (to me).
If there's agreement on this I could look to do that change as a follow up...

Alternatively, for those callers that do not need to use c_str() and don't care
about trailing \0 (i.e. just dealing in raw binary data, not strings), we could
just change them to use resize() and string_as_array().

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h#newcode472
base/string_util.h:472: return &((*str)[0]);
Maybe while you're here, the standards-approved way of doing this is:-
return &*str->begin();

see the comment on string_as_array in
http://src.chromium.org/viewvc/chrome/trunk/src/base/stl_util.h?revision=9335...
-- and also note the ISO/IEC 14882:2008 spec included the proposed wording about
this (see the second occurrence of 'contiguously' in
http://goto.google.com/twlli

http://codereview.chromium.org/8418034/diff/7005/net/disk_cache/entry_impl.cc
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/8418034/diff/7005/net/disk_cache/entry_impl.cc...
net/disk_cache/entry_impl.cc:776: ++key_len;  // We store a trailing \0 on disk
that we read back below.
My reading of line 432 is we don't write the trailing \0. So odds are we're
actually reading a spurious junk byte in here? (It's 'safely' ignored as it's
over the end of size; a subsequent c_str() call would overwrite it with \0
anyway)

[cbentzel, 2011-11-01 12:15:06]

Thank you for cleaning the mess I made. I agree that this is an improvement over
returning NULL.

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h#newcode451
base/string_util.h:451: // convenient in that is can be used inline in the call,
and fast in that it
I'm not sure about the fast comment.

With WriteInto, we do a zero-fill because of the resize(), followed by writing
directly into the buffer.

With a char* followed by Assign, we'd copy into the char* buffer followed by a
string copy.

These are likely about the same - but I haven't measured. The char* followed by
Assign may also have unneeded allocation and free for the char* [such as in a
scoped_array] which would be worse than WriteInto.

http://codereview.chromium.org/8418034/diff/7005/ui/base/win/ime_input.cc
File ui/base/win/ime_input.cc (right):

http://codereview.chromium.org/8418034/diff/7005/ui/base/win/ime_input.cc#new...
ui/base/win/ime_input.cc:346: if (!lparam || !type)
This isn't the same logic as before - intentional?

[rvargas (doing something else), 2011-11-01 18:13:47]

http://codereview.chromium.org/8418034/diff/7005/net/disk_cache/entry_impl.cc
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/8418034/diff/7005/net/disk_cache/entry_impl.cc...
net/disk_cache/entry_impl.cc:776: ++key_len;  // We store a trailing \0 on disk
that we read back below.
On 2011/11/01 11:25:32, joth wrote:
> My reading of line 432 is we don't write the trailing \0. So odds are we're
> actually reading a spurious junk byte in here? (It's 'safely' ignored as it's
> over the end of size; a subsequent c_str() call would overwrite it with \0
> anyway)

The extra \0 on disk comes from lines 421 and 438.

[joth, 2011-11-01 19:14:50]

http://codereview.chromium.org/8418034/diff/7005/net/disk_cache/entry_impl.cc
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/8418034/diff/7005/net/disk_cache/entry_impl.cc...
net/disk_cache/entry_impl.cc:776: ++key_len;  // We store a trailing \0 on disk
that we read back below.
On 2011/11/01 18:13:48, rvargas wrote:
> On 2011/11/01 11:25:32, joth wrote:
> > My reading of line 432 is we don't write the trailing \0. So odds are we're
> > actually reading a spurious junk byte in here? (It's 'safely' ignored as
it's
> > over the end of size; a subsequent c_str() call would overwrite it with \0
> > anyway)
> 
> The extra \0 on disk comes from lines 421 and 438.

Ah yes, thanks.
Still - seems a bit funny this is intentionally reading off the end of
key_.size() one extra byte that is inaccessible by any conventional means be 
(both c_str() and resize(size()+1) will overwrite it)

[rvargas (doing something else), 2011-11-01 19:36:52]

http://codereview.chromium.org/8418034/diff/7005/net/disk_cache/entry_impl.cc
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/8418034/diff/7005/net/disk_cache/entry_impl.cc...
net/disk_cache/entry_impl.cc:776: ++key_len;  // We store a trailing \0 on disk
that we read back below.
On 2011/11/01 19:14:51, joth wrote:
> On 2011/11/01 18:13:48, rvargas wrote:
> > On 2011/11/01 11:25:32, joth wrote:
> > > My reading of line 432 is we don't write the trailing \0. So odds are
we're
> > > actually reading a spurious junk byte in here? (It's 'safely' ignored as
> it's
> > > over the end of size; a subsequent c_str() call would overwrite it with \0
> > > anyway)
> > 
> > The extra \0 on disk comes from lines 421 and 438.
> 
> Ah yes, thanks.
> Still - seems a bit funny this is intentionally reading off the end of
> key_.size() one extra byte that is inaccessible by any conventional means be 
> (both c_str() and resize(size()+1) will overwrite it)

yes, that looks like an oversight. I think the original intent was to double
check there was a \0 there... oh well.

[Peter Kasting, 2011-11-01 19:43:27]

New snap up.  This is now ready for review.

jshin: net_util.cc
cbentzel: rest... I contacted individual owners on any place I wasn't sure, so
if you're concerned about anything, I can either relay their messages or ask
them to comment here
mark: OWNERS

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h
File base/string_util.h (left):

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h#oldcode478
base/string_util.h:478: return NULL;
On 2011/11/01 11:25:32, joth wrote:
> - in the case someone does access the return value it turns a 'simple' NULL
> pointer deref into a platform-dependent undefined behavior. (Of course, we
hope
> the DCHECK does catch it before that goes into a release)

And that would violate our "do not handle assertion failures" style rule.

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h#newcode451
base/string_util.h:451: // convenient in that is can be used inline in the call,
and fast in that it
On 2011/11/01 12:15:07, cbentzel wrote:
> These are likely about the same - but I haven't measured.

Mostly I was trying to restore some of the detail from the original comments on
this function.

I don't have violent feelings about the "fast" part, but I think at worst this
will be the same speed as the pattern you describe, and in some situations
better.  The real intent of the comment was more to indicate that we weren't
aware of an even better way to do this sort of access at the time we wrote the
function.

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h#newcode468
base/string_util.h:468: size_t length_with_null) {
On 2011/11/01 11:25:32, joth wrote:
> completely orthogonal issue to the one you're tackling here, but do the
majority
> of callers have to explicitly +1 to there argument? I'd find this API a lot
more
> intuitive if the size I pass in was the same as the size of the string after
the
> call, and hence the same as the number of bytes I am then permitted to write
> into it.

But the size of the string after the call and the number of bytes you are
permitted to write in are NOT the same.  And they can't be the same if you want
to support APIs that write a C-style string + trailing '\0' which was the
original point of WriteInto (since expanded to be used in more cases). 
Otherwise, callers either could not write the trailing byte, or they could write
it and it would be included in the string itself.

> The + 1 for null is really just an internal optimization, to remove the risk a
> subsequent call to c_str() has to incur a reallocation. It doesn't seem to
have
> any bearing on the observable contract (to me).

Not true, see above.

http://codereview.chromium.org/8418034/diff/7005/base/string_util.h#newcode472
base/string_util.h:472: return &((*str)[0]);
On 2011/11/01 11:25:32, joth wrote:
> Maybe while you're here, the standards-approved way of doing this is:-
> return &*str->begin();
> 
> see the comment on string_as_array in
>
http://src.chromium.org/viewvc/chrome/trunk/src/base/stl_util.h?revision=9335...
> -- and also note the ISO/IEC 14882:2008 spec included the proposed wording
about
> this (see the second occurrence of 'contiguously' in
> http://goto.google.com/twlli

I see the standard using this as an example of an identity that holds, but I
don't see it saying that what we're doing is in any sense wrong; in fact I read
the standard as guaranteeing that our form is equivalent.  See section 21.3.5
item 1 which says that operator[](pos) returns *(begin() + pos).

http://codereview.chromium.org/8418034/diff/7005/ui/base/win/ime_input.cc
File ui/base/win/ime_input.cc (right):

http://codereview.chromium.org/8418034/diff/7005/ui/base/win/ime_input.cc#new...
ui/base/win/ime_input.cc:346: if (!lparam || !type)
On 2011/11/01 12:15:07, cbentzel wrote:
> This isn't the same logic as before - intentional?

Nice catch!  I misread "&" as "&&".

[jungshik at Google, 2011-11-01 20:13:09]

http://codereview.chromium.org/8418034/diff/13001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/8418034/diff/13001/net/base/net_util.cc#newcod...
net/base/net_util.cc:231: length, &err);
|length| is now the target buffer capacity. The 2nd last argument is the source
length. So, the 2nd |length| when calling ucnv_toAlgorithmic should be now
decoded.length().

[Peter Kasting, 2011-11-01 20:21:51]

http://codereview.chromium.org/8418034/diff/13001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/8418034/diff/13001/net/base/net_util.cc#newcod...
net/base/net_util.cc:231: length, &err);
On 2011/11/01 20:13:09, Jungshik Shin wrote:
> |length| is now the target buffer capacity. The 2nd last argument is the
source
> length. So, the 2nd |length| when calling ucnv_toAlgorithmic should be now
> decoded.length().  

Fixed.  Renamed this |output_length| for clarity.

[jungshik at Google, 2011-11-01 20:39:46]

thanks. net_util : lgtm 

On 2011/11/01 20:21:51, Peter Kasting wrote:
> http://codereview.chromium.org/8418034/diff/13001/net/base/net_util.cc
> File net/base/net_util.cc (right):
> 
>
http://codereview.chromium.org/8418034/diff/13001/net/base/net_util.cc#newcod...
> net/base/net_util.cc:231: length, &err);
> On 2011/11/01 20:13:09, Jungshik Shin wrote:
> > |length| is now the target buffer capacity. The 2nd last argument is the
> source
> > length. So, the 2nd |length| when calling ucnv_toAlgorithmic should be now
> > decoded.length().  
> 
> Fixed.  Renamed this |output_length| for clarity.

[Peter Kasting, 2011-11-01 22:33:56]

+rsleevi.  Ryan, can you either LG or tell me to raise certain DCHECKs to CHECKs
for all encryptor* files?  This includes the changes I emailed you about last
week, and I've also changed encryptor_nss.cc to match the DCHECKs of the other
implementations (and documented the assertions in encryptor.h).

[Ryan Sleevi, 2011-11-01 23:08:58]

To make sure I'm understanding this change:

Currently, if WriteInto(..., 0 || 1) is called, NULL will be returned, and any
attempts to deref/store should generally cause an exception/crash.

With this change, it will return... (something potentially valid? still crash?),
so it's up to the DCHECKs to catch this. In Release mode though, without those
DCHECKs, there will be no sanity checking before attempting to write.

Given that the Encryptor is exposed to potentially hostile input, and if the
buffer retuned by WriteInto might appear valid, I'd be inclined to promote all
of the size checks to CHECKs - at least until a WARN_UNUSUED_RESULT sweep can be
made on Encryptor.

Given that turning on WUR for HMAC resulted in finding a number of places where
the API was being misused, at least CHECK()ing will give us the same guarantees
that are implicit in attempting to deref a NULL returned via WriteInto.

Or you could just promote the WriteInto DCHECK into a CHECK, to provide the same
(safety) guarantee.

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:53: DCHECK_GT(output_size + 1, input.size());
Both of these can be changed to CHECK_GT

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_nss.cc
File crypto/encryptor_nss.cc (right):

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_nss.cc#new...
crypto/encryptor_nss.cc:111: DCHECK_GT(output_len, input.size());
CHECK_GT

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_nss.cc#new...
crypto/encryptor_nss.cc:158: DCHECK_GE(output_len, input.size());
CHECK_GE

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_openssl.cc
File crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_openssl.cc...
crypto/encryptor_openssl.cc:112: DCHECK_GT(output_size + 1, input.size());
Promote both of these to CHECK_GT

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_win.cc
File crypto/encryptor_win.cc (right):

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_win.cc#new...
crypto/encryptor_win.cc:89: DCHECK_GT(total_len + 1, data_len);
86, 88, 89 -> CHECK variants

http://codereview.chromium.org/8418034/diff/11042/crypto/encryptor_win.cc#new...
crypto/encryptor_win.cc:111: DCHECK_GT(data_len + 1, data_len);
both of these to CHECK_GT

[Peter Kasting, 2011-11-01 23:20:16]

On 2011/11/01 23:08:58, Ryan Sleevi wrote:
> Currently, if WriteInto(..., 0 || 1) is called, NULL will be returned, and any
> attempts to deref/store should generally cause an exception/crash.
> 
> With this change, it will return... (something potentially valid? still
crash?),
> so it's up to the DCHECKs to catch this. In Release mode though, without those
> DCHECKs, there will be no sanity checking before attempting to write.

The DCHECKs are saying that no one will ever call WriteInto() with 0 or 1 for a
length, regardless of circumstance.  We don't do further checks (e.g. in release
mode) because we should almost never handle assertion failures (see style
guide).

> Given that the Encryptor is exposed to potentially hostile input, and if the
> buffer retuned by WriteInto might appear valid, I'd be inclined to promote all
> of the size checks to CHECKs - at least until a WARN_UNUSUED_RESULT sweep can
be
> made on Encryptor.

If Encryptor is exposed to hostile input in the sense that some of what I'm
DCHECKing against could actually ever happen, then we need to neither DCHECK nor
CHECK, but actually handle that input via conditionals (presumably by returning
false).

The DCHECKs I added are saying "we cannot ever violate this, even if we're being
attacked".  Either that's correct or it's not.  If it's correct, CHECK doesn't
help.  If it's not, CHECK isn't sufficient.

We should very rarely if ever be using CHECK in our code, mostly just to track
down crashes.  I guess you can consider my question about raising to CHECK to
really be a question about converting to a conditional, because I don't think
CHECK is actually appropriate here.  I will do it if you insist but I want you
to be really really sure because I would never write these as CHECKs myself or
ask for them to be CHECKs as a reviewer.  I DEFINITELY would not just blindly
use CHECK everywhere in this code as paranoia.

[Ryan Sleevi, 2011-11-01 23:30:23]

On 2011/11/01 23:20:16, Peter Kasting wrote:
> On 2011/11/01 23:08:58, Ryan Sleevi wrote:
> > Given that the Encryptor is exposed to potentially hostile input, and if the
> > buffer retuned by WriteInto might appear valid, I'd be inclined to promote
all
> > of the size checks to CHECKs - at least until a WARN_UNUSUED_RESULT sweep
can
> be
> > made on Encryptor.
> 
> If Encryptor is exposed to hostile input in the sense that some of what I'm
> DCHECKing against could actually ever happen, then we need to neither DCHECK
nor
> CHECK, but actually handle that input via conditionals (presumably by
returning
> false).
> 
> The DCHECKs I added are saying "we cannot ever violate this, even if we're
being
> attacked".  Either that's correct or it's not.  If it's correct, CHECK doesn't
> help.  If it's not, CHECK isn't sufficient.
> 
> We should very rarely if ever be using CHECK in our code, mostly just to track
> down crashes.  I guess you can consider my question about raising to CHECK to
> really be a question about converting to a conditional, because I don't think
> CHECK is actually appropriate here.  I will do it if you insist but I want you
> to be really really sure because I would never write these as CHECKs myself or
> ask for them to be CHECKs as a reviewer.  I DEFINITELY would not just blindly
> use CHECK everywhere in this code as paranoia.

To be clear: I absolutely agree that this should be handled as a defined part of
the API through explicit checks (lower-case) and well-defined error returns (eg:
return false). That's not a question.

In an ideal world (which hopefully the state of the code is close to), every
caller/consumer of Encryptor is checking the results before performing further
actions. This is especially important if doing something like re-using a buffer,
since failing to check a return for something like Encrypt()/Decrypt() could
lead to plaintext or keying material being disclosed.

However, I don't have the confidence or the knowledge to say that every caller
is respecting these. I provided an example of a different class (crypto/hmac.h)
that was definitively NOT being used correctly.

Currently, the most egregious misuses of this API are implicitly 'protected' by
the fact that NULL will be returned, thus *NULL is likely, thus a crash. This CL
weakens that from a crash to a DCHECK or implementation-defined behaviour.
Because of my experience with HMAC, I don't think that is a safe weakening
unless we can guarantee there are no misuses of Encryptor, such as by
WARN_UNUSED_RESULT and auditing the code. If there are no misuses, then it's
safe to change all of the DCHECKs into actual conditionals and return false.

However, I think it's unreasonable to ask you to do that as a precursor to
committing what is largely unrelated, thus I'm asking that you do not weaken the
guarantees for crypto/, by using CHECK, until that can happen.

Does that help explain why I'm so keen on CHECK?

[Peter Kasting, 2011-11-02 00:01:37]

On 2011/11/01 23:30:23, Ryan Sleevi wrote:
> To be clear: I absolutely agree that this should be handled as a defined part
of
> the API through explicit checks (lower-case) and well-defined error returns
(eg:
> return false). That's not a question.

It seems like you view an API that handles any input and simply returns true or
false as superior to one that has preconditions that it asserts.  But IMO that
situation is much worse because it is harder to reason about and because callers
have to do much more error-handling (as you have noted that they are unlikely to
correctly do).  Therefore, it is vastly better for these APIs to simply DCHECK,
and we should only fall back on conditionals if we can't tell what their callers
actually guarantee.  We certainly shouldn't aspire to conditionals!

> Currently, the most egregious misuses of this API are implicitly 'protected'
by
> the fact that NULL will be returned, thus *NULL is likely, thus a crash.

There are several problems with this statement:

(1) We only added the NULL return to WriteInto() two weeks ago.  If you think
the WriteInto behavior has been protecting you in any public releases, you're
mistaken.  From 2006 until two weeks ago it looked just like what I'm changing
it back to except sans DCHECK.

(2) WriteInto() only returns NULL when passed an argument with length 1.  The
DCHECKs in question here deal with other cases as well, e.g. overflow.  Those
aren't scenarios where we'd return NULL and you'd be "saved by a crash" anyway.

In sum, it has never been true in the past, and in my view isn't even true now,
that "the most egregious misuses will result in a quick NULL deref".  Rather, if
there are really problems of the kind you don't think exist but are worried
might anyway, then they've been problematic for arbitrarily large amounts of
time.

> If there are no misuses, then it's
> safe to change all of the DCHECKs into actual conditionals and return false.

Again, we should not aspire to ever have conditionals here.  Having DCHECKs is
the best state for the code to be in permanently.

For now, if you think callers are all respecting the preconditions specified in
this change, I think we should just change everything to DCHECK and file a bug
to audit the code.  This won't make anything worse than it already is.  If
they're not, then there is no good answer, because CHECKs will cause crashes
that we don't want, and you're worried that conditionals won't be handled
correctly.  It's impossible to reason about code with this level of uncertainty.

Does this help explain why I'm so opposed to CHECK?

[Ryan Sleevi, 2011-11-02 00:55:51]

On 2011/11/02 00:01:37, Peter Kasting wrote:
> On 2011/11/01 23:30:23, Ryan Sleevi wrote:
> > To be clear: I absolutely agree that this should be handled as a defined
part
> of
> > the API through explicit checks (lower-case) and well-defined error returns
> (eg:
> > return false). That's not a question.
> 
> It seems like you view an API that handles any input and simply returns true
or
> false as superior to one that has preconditions that it asserts.  But IMO that
> situation is much worse because it is harder to reason about and because
callers
> have to do much more error-handling (as you have noted that they are unlikely
to
> correctly do).  Therefore, it is vastly better for these APIs to simply
DCHECK,
> and we should only fall back on conditionals if we can't tell what their
callers
> actually guarantee.  We certainly shouldn't aspire to conditionals!

This seems counter to what the Chromium Coding Style says. "Sometimes it is
preferable to force a crash to happen via CHECK. Those cases include situations
where you are dealing with untrusted input (provided there is no reasonable
recovery path) and the consequences of continuing execution could result in a
security vulnerability."

I view a misuse of classes in crypto/ as potential security vulnerabilities.
Certainly as needing to be resilient against untrusted input - after all, it may
be the crypto/ code itself that is determining whether or not input IS trusted
to begin with (via decryption or mac-verification). This was true for the HMAC
code, though it never made it into shipping.

I think the rest of the discussion hinges on whether this is viewed as an edge
piece (directly exposed to user input) or an inner/core piece (all inputs must
be sanitized beforehand). If it's an inner piece, then arguably DCHECK() is
correct, as it's a sign that the caller has not properly validated the inputs.
If it's an edge piece, then conditionals really are correct - the caller is
properly using the API, and the API is properly rejecting invalid inputs, it's
the "hostile" source who is at fault, and we shouldn't bend or break for hostile
input.

I view this as an edge piece - at the frontline of potentially hostile input. It
sounds like you view this as inner - where it is the caller's responsibility to
sanitize all inputs before hand.

If you go the DCHECK route, then someone, somewhere, still has to perform checks
for ciphertext.empty() or plaintext.size() > 0 or plaintext.size <
std::numeric_limits<size_t>::max() before calling the API, when receiving user
input. Those are important, potentially security sensitive checks. Having them
scattered at every call-site can make them easy to get wrong, and a failure to
implement them correctly is /no/ guarantee that the DCHECK will fire in any of
the existing tests. 

From the design of this class (and those in crypto/), they MUST be able to
handle malformed input gracefully. Malformed input is one of the many ways you
attack cryptosystems, and so the API needs to be built in such a way that it
performs adequate checks and reveals as little as possible (ideally, nothing).


I don't believe the callers are respecting the conditions, nor do I believe they
should be responsible for enforcing them on inputs, thus I do support filing a
bug. This CL strengthens the checks for some implementations (by adding a
DCHECK() for bounds) and weakens it on others (arguably the most deployed
platform, Windows, which currently CHECKs). I'm not convinced DCHECK is correct
here, but if that's the view shared by other reviewers, then I'll learn.

I'm not convinced that because we used to not check the inputs (or could
potentially write to NULL) that it's safe not to check the inputs, but that
isn't to say it may not be 'acceptable'. For that, I'd defer to the reviewers
here, and if you want a crypto/ OWNER, look for wtc. I know my defensiveness (in
a "defensive programming" sense) tends to be much higher, and while I hope that
it's not leading me to be short-sighted, I'm not convinced this is the
best/correct approach. wtc may be able to offer a differing opinion, and
especially greater familiarity with the "Chromium-y" way and style for
situations such as this.

The previous discussion related to the problems here, and reasoning about it, is
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
. An example of a past bug is http://crbug.com/90802 - and the HMAC example I
keep bringing up was http://crrev.com/94826

[Peter Kasting, 2011-11-02 01:17:41]

On 2011/11/02 00:55:51, Ryan Sleevi wrote:
> I think the rest of the discussion hinges on whether this is viewed as an edge
> piece (directly exposed to user input) or an inner/core piece (all inputs must
> be sanitized beforehand). If it's an inner piece, then arguably DCHECK() is
> correct, as it's a sign that the caller has not properly validated the inputs.
> If it's an edge piece, then conditionals really are correct - the caller is
> properly using the API, and the API is properly rejecting invalid inputs, it's
> the "hostile" source who is at fault, and we shouldn't bend or break for
hostile
> input.

Yes.  This distinction is precisely what I was trying to articulate in asking
you  whether DCHECKs or conditionals were more appropriate.

> I view this as an edge piece - at the frontline of potentially hostile input.
It
> sounds like you view this as inner - where it is the caller's responsibility
to
> sanitize all inputs before hand.

No, I don't.  I don't know -- or care -- which one this is.  My comments
regarding conditionals being something not to aspire to were based on my
understanding of you as claiming that these functions were, in fact, "inner"
pieces that should have already had validation applied elsewhere.

If that's not what you were saying, and these functions are actually supposed to
validate third-party input, then all the things I'm currently DCHECKing should
be conditionals instead.

What I mostly want from you is a definitive answer as to which one these are. 
Are they definitely "edge" pieces, or do you just suspect that they might be? 
Who knows for sure where the API contract draws its validation lines?

> From the design of this class (and those in crypto/), they MUST be able to
> handle malformed input gracefully. Malformed input is one of the many ways you
> attack cryptosystems, and so the API needs to be built in such a way that it
> performs adequate checks and reveals as little as possible (ideally, nothing).

Again, I don't know whether you're speaking about the Encrypt() and Decrypt()
API calls here, or about the larger system of code surrounding them.  From what
you said above, I take it it's the former, but I'm not sure (and I'm not sure
you're sure?).

> I'm not convinced that because we used to not check the inputs (or could
> potentially write to NULL) that it's safe not to check the inputs, but that
> isn't to say it may not be 'acceptable'. For that, I'd defer to the reviewers
> here, and if you want a crypto/ OWNER, look for wtc. I know my defensiveness
(in
> a "defensive programming" sense) tends to be much higher, and while I hope
that
> it's not leading me to be short-sighted, I'm not convinced this is the
> best/correct approach. wtc may be able to offer a differing opinion, and
> especially greater familiarity with the "Chromium-y" way and style for
> situations such as this.

It seems clear we keep misunderstanding each other.  I'll assume you're saying
that these calls are "edge" functions that must validate user data.  That's
fine, but in that case we can't CHECK because that means that Chrome will crash
any time we get input that breaks the CHECKs.  Crashing is a P0 problem, not
something we should ever intentionally allow.  The cases in the style guide
where we CHECK "to prevent a security problem" are cases that are not supposed
to be reachable and are e.g. defense at the process boundary line against a
compromised renderer, where we're absolutely hosed and there's no conceivable
bail-out.  That isn't true here.  Here, the right thing to do is to simply
return false.

You also seem to be worried that if we return false some caller might not do
something right and there might be some kind of problem.  This is too vague to
reason about, and in particular it's too vague for me to say that it's a worse
problem than causing Chrome to crash every time a website gives us some series
of bytes.  If we know of specific security compromises that are possible, we can
try and work around them.  If we're worried, we can file a bug to audit code. 
But in the absence of specifics, I don't think we should just make all of these
things that should really be conditionals be CHECKs, because then we're doing
something we KNOW is wrong and can crash in order to avoid something that MIGHT
be wrong and could... something.

I'm sorry that we keep talking past each other and I'm probably driving you
crazy.  I really don't mean to be.  I want to do the right thing here.

[Ryan Sleevi, 2011-11-02 02:04:26]

On 2011/11/02 01:17:41, Peter Kasting wrote:
> What I mostly want from you is a definitive answer as to which one these are. 
> Are they definitely "edge" pieces, or do you just suspect that they might be? 
> Who knows for sure where the API contract draws its validation lines?

hmac.*, rsa_private_key.*, and signature_verifier.* are all 'edge' pieces
directly exposed to attacker/user-controlled input at some point during their
lifetimes (most notably: extensions), that cannot be validated by anything BUT
these classes - since 'good' input should be indistinguishable from 'bad' input
(or for 'random' input) for everybody BUT these classes.

I view this class being the same - hostile input in, graceful "Shut up and go
away" out.

The reason I view this (and in general, crypto/) code is that most guarantees of
crypto is that whatever magic output should be indistinguishable from
randomness. Thus, even the absurdist, weird, crazy pathological edge cases may
be perfectly 'normal' under some strange condition (CBC vs CTR, padding modes,
etc). Caller's shouldn't have to be responsible to know these.

> 
> Again, I don't know whether you're speaking about the Encrypt() and Decrypt()
> API calls here, or about the larger system of code surrounding them.  From
what
> you said above, I take it it's the former, but I'm not sure (and I'm not sure
> you're sure?).

Can I cheat and say "Both" ? The larger system of code within crypto/, but
Init()/Encrypt()/Decrypt() in particular.

> It seems clear we keep misunderstanding each other.  I'll assume you're saying
> that these calls are "edge" functions that must validate user data.  That's
> fine, but in that case we can't CHECK because that means that Chrome will
crash
> any time we get input that breaks the CHECKs.  Crashing is a P0 problem, not
> something we should ever intentionally allow.  The cases in the style guide
> where we CHECK "to prevent a security problem" are cases that are not supposed
> to be reachable and are e.g. defense at the process boundary line against a
> compromised renderer, where we're absolutely hosed and there's no conceivable
> bail-out.  That isn't true here.  Here, the right thing to do is to simply
> return false.

Right. I agree that the /correct/ thing from an API perspective to do is to
return false for every one of those DCHECKs. This is what I want to see the API
doing.

What I'm concerned is an issue like
http://src.chromium.org/viewvc/chrome/trunk/src/remoting/protocol/secure_p2p_...
(Lines 260 - 270), not properly checking failures, and then continuing to use
inputs/outputs in a manner the attacker can influence.

This is a pre-existing concern. My (likely misguided) assumption when I looked
at this (Encryptor) code, when addressing the HMAC code, is that we "should"
crash before here. That may not be the case though.

In theory, each of the Encryptor implementations' returning false for
(En/De)crypt should leave no 'secrets' hanging about - nor should |output|
reflect any lengths. This isn't always true - eg: encryptor_nss.cc resizes
|output| for CTR, even during failure. This means that if a Decrypt() failed,
but wasn't checked, it would still be valid (look ma, no crashes) to check
output.size() and even access the data - as if it was a zeroed buffer. Could
that lead to security vulnerabilities? Only an audit would say - I hope not, but
I cannot /say/ it won't.

What I see as the 'correct' tasks:
 - Slap some WARN_UNUSED_RESULTs to make sure the API returns are being checked
 - Ensure for each of the implementations, they're checking things like ranges
(which this CL addresses by way of DCHECK, but as we've discussed, should be
conditionals)
  - Ensure each of the implementations has the same pre and post-conditional
guarantees
  - Profit

It does/did not seem reasonable to ask the above of you in this CL, or as a
precondition, and I don't see myself getting to this for at least another week,
at the earliest.

In theory, these conditions should represent edge cases that shouldn't be
reachable (eg: overflowing a size_t is hard, even with untrusted inputs), and
thus may be 'mostly' academic/defensive/paranoid. In the absence of doing the
above and fixing it /right/, I wanted the CHECK() because it immediately
surfaces if there is a problem (via crash reports) AND it guarantees that, for
whatever the implementation does, it shouldn't 'leak' or misbehave further.

In an pragmatic world, these CHECKs() are only in for a week or two, will
(hopefully) never be hit, and will be removed once the code can be audited
(WARN_UNUSED_RESULT & git grep ftw). That's why I wanted to give you an 'out' to
land this CL with /just/ using CHECK.

Using DCHECK leaves me nervous (but perhaps unreasonably so), because of
problems such as the HMAC one (dead horse: thoroughly beaten), and because of
situations like CryptCTR potentially having good-looking side-effects even in
failure (literally, that was the first file I looked at, and it stood out right
away). Perhaps it's no worse than what we're already shipping, but if we've at
least identified a point of weakness, I'd rather put /some/ pragmatic defenses
in (which I view CHECKing as, for this specific code, for what it is supposed to
do and guarantee) than just leave it alone.

> 
> I'm sorry that we keep talking past each other and I'm probably driving you
> crazy.  I really don't mean to be.  I want to do the right thing here.

No, I think it's perfectly understandable given the medium we're using. I
equally want to make sure I'm not donning too thick a tinfoil hat in my
paranoia/"defensive coding", nor sounding too "crash them all and let God sort
them out" in my approach here.

[Peter Kasting, 2011-11-02 18:00:09]

OK, you convinced me.  I'll change these all to CHECKs and then file a separate
bug to cover whatever auditing needs to happen to convert them all to
conditionals.  Thanks for your patience.

[Peter Kasting, 2011-11-02 21:10:02]

On 2011/11/02 18:00:09, Peter Kasting wrote:
> OK, you convinced me.  I'll change these all to CHECKs and then file a
separate
> bug to cover whatever auditing needs to happen to convert them all to
> conditionals.

New snap up that uses CHECK.  Filed http://crbug.com/102722 about the auditing.

[Peter Kasting, 2011-11-15 00:27:32]

cbentzel: ping?
rsleevi: Given http://codereview.chromium.org/8511050/ , should I revert all my
changes to crypto/?  Or are some of the changes in there not covered by your
changes?

[Ryan Sleevi, 2011-11-15 00:30:09]

On 2011/11/15 00:27:32, Peter Kasting wrote:
> cbentzel: ping?
> rsleevi: Given http://codereview.chromium.org/8511050/ , should I revert all
my
> changes to crypto/?  Or are some of the changes in there not covered by your
> changes?

pkasting: All of your changes/checks to crypto/ are incorporated in the
referenced CL, including 0/size+1 checking.

[cbentzel, 2011-11-15 11:37:09]

LGTM

Remove the empty BUG=/TEST= lines from the CL description.

I did not review the crypto files since rsleevi is on top of those.

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:29: DCHECK_GT(executable_length, 1u);
Is this guaranteed? Should "return false" be used if executable_length == 0?

http://codereview.chromium.org/8418034/diff/13022/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/8418034/diff/13022/net/base/net_util.cc#newcod...
net/base/net_util.cc:164: const size_t last_good_char =
input.find_last_not_of(trailing_chars);
Should this stay size_type? find_last_not_of returns a size_type rather than a
size_t.

http://codereview.chromium.org/8418034/diff/13022/net/base/net_util.cc#newcod...
net/base/net_util.cc:210: if (!((enc_type == B_ENCODING) ?
I find the old logic easier to parse than this ternary, but this is still
correct.

[Peter Kasting, 2011-11-15 18:50:32]

On 2011/11/15 11:37:09, cbentzel wrote:
> Remove the empty BUG=/TEST= lines from the CL description.

They're not empty, they say "none".

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:29: DCHECK_GT(executable_length, 1u);
On 2011/11/15 11:37:10, cbentzel wrote:
> Is this guaranteed? Should "return false" be used if executable_length == 0?

The old code already DCHECKed that the length wasn't zero.  I'm just checking
that it isn't one either.  I can't see how the full path to the executable could
possibly contain one character (even if the executable itself were named
something one character long).

http://codereview.chromium.org/8418034/diff/13022/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/8418034/diff/13022/net/base/net_util.cc#newcod...
net/base/net_util.cc:164: const size_t last_good_char =
input.find_last_not_of(trailing_chars);
On 2011/11/15 11:37:10, cbentzel wrote:
> Should this stay size_type? find_last_not_of returns a size_type rather than a
> size_t. 

size_type is banned in favor of size_t per the Chromium style guide. 
Practically speaking the two are always the same for std::string
implementations.

[Peter Kasting, 2011-11-28 22:31:16]

Can I get OWNERS r+ from:

mark: base
rsleevi: crypto -- since http://codereview.chromium.org/8511050/ has not been
landed yet it seems appropriate to go ahead and land the crypto changes in this
patch rather than reverting them, I think?
sky: ui/base, views

[Mark Mentovai, 2011-11-28 22:41:20]

LG otherwise in base.

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:29: DCHECK_GT(executable_length, 1u);
Peter Kasting wrote:
> The old code already DCHECKed that the length wasn't zero.  I'm just checking
> that it isn't one either.  I can't see how the full path to the executable
could
> possibly contain one character (even if the executable itself were named
> something one character long).

The call you’re checking has a break-down case in which it might not return the
full path to the executable, but just the leaf filename. But it’s returning the
required buffer size with a terminator, so the executable would have to be empty
for executable_length to be 1, therefore this ought to be fine.

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:35: return true;
As written now, is there even a good reason for this to have a bool return type?
That just seems to indicate that it might sometimes return false, but it never
does.

http://codereview.chromium.org/8418034/diff/13022/base/string_util_unittest.cc
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/8418034/diff/13022/base/string_util_unittest.c...
base/string_util_unittest.cc:1084: void WritesCorrectly(size_t num_chars) {
Can be static?

[scherkus (not reviewing), 2011-11-28 23:05:21]

media LGTM

[sky, 2011-11-28 23:48:53]

LGTM

[Peter Kasting, 2011-11-29 01:48:15]

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/8418034/diff/13022/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:35: return true;
On 2011/11/28 22:41:21, Mark Mentovai wrote:
> As written now, is there even a good reason for this to have a bool return
type?
> That just seems to indicate that it might sometimes return false, but it never
> does.

I agree with you.  Changed to void.

http://codereview.chromium.org/8418034/diff/13022/base/string_util_unittest.cc
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/8418034/diff/13022/base/string_util_unittest.c...
base/string_util_unittest.cc:1084: void WritesCorrectly(size_t num_chars) {
On 2011/11/28 22:41:21, Mark Mentovai wrote:
> Can be static?

Done.

[Ryan Sleevi, 2011-11-29 02:32:32]

crypto/ LGTM

[Mark Mentovai, 2011-11-29 19:23:11]

LGTM per harassment