This adds support for encrypted ONC import

This adds support for encrypted ONC import to Chrome.

We now can import standalone ONC files that are encrypted by the
Spigots management app.

TBR=joaodasilva@chromium.org
BUG=chromium-os:19397
TEST=Ran new unit tests, imported encrypted ONC on device.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117321

[Charlie Lee, 2011-12-22 20:55:15]

lgtm with nits

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:250: return false;
should add a log warning here for why this would fail.

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:394: if
(parse_error_.empty())
when would this not be empty?

[Ryan Sleevi, 2011-12-24 07:34:08]

I'd like to raise some security concerns with this implementation, in the hopes
of double-checking that this really is intentional - and perhaps changing the
ONC format before it becomes a shipping thing.

You may want to reach out to Adam Langley (agl@google) or Ben Laurie
(benl@google), as my understanding is that they both handle reviews of this
sort.

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:202: const
crypto::SymmetricKey* key) {
See crypto::HMAC::Verify()

Same implementation, but re-uses the existing code used elsewhere.

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:247: if (std::string(digest,
length) == hmac)
See above comment. This comparison is not a constant time comparison, therefore
it leaks timing information as a side-channel.

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:370: // Split out the HMAC.
As a drive-by, if it's not too late, wouldn't this make more sense from a
cryptographic security principal to have the HMAC cover the encrypted
ciphertext, rather than be encryptd along with the plaintext?

A good understanding of why this is can be had at
http://blog.thoughtcrime.org/the-cryptographic-doom-principle

If this is a new format being developed at Google (which, as best I can tell, it
is - at least the design docs are all restricted to .google.com), then perhaps
there's the opportunity to avoid some of the common pitfalls here.

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:384:
IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_HMAC_CHECK_FAILED);
The fact that HMAC errors are distinguished from encryption errors can be used
to potentially mount a Vaudenay attack. At the least, there is some timing
information being leaked here about which failed.

[Greg Spencer (Chromium), 2012-01-03 19:40:12]

On 2011/12/24 07:34:08, Ryan Sleevi wrote:
> You may want to reach out to Adam Langley (agl@google) or Ben Laurie
> (benl@google), as my understanding is that they both handle reviews of this
> sort.

Yes, I was planning on asking Adam to take a look after passing "regular" code
review.

>
http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
> File chrome/browser/chromeos/cros/onc_network_parser.cc (right):
> 
>
http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
> chrome/browser/chromeos/cros/onc_network_parser.cc:202: const
> crypto::SymmetricKey* key) {
> See crypto::HMAC::Verify()
> 
> Same implementation, but re-uses the existing code used elsewhere.
> 
>
http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
> chrome/browser/chromeos/cros/onc_network_parser.cc:247: if
(std::string(digest,
> length) == hmac)
> See above comment. This comparison is not a constant time comparison,
therefore
> it leaks timing information as a side-channel.

Done.  Thanks, I wasn't aware of the separate HMAC class.  I also added an Init
function to the HMAC class that takes a SymmetricKey since using the HMAC class
with a SymmetricKey requires getting the raw key, and that's error prone (have
to clear the key after fetching it).

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
> chrome/browser/chromeos/cros/onc_network_parser.cc:370: // Split out the HMAC.
> As a drive-by, if it's not too late, wouldn't this make more sense from a
> cryptographic security principal to have the HMAC cover the encrypted
> ciphertext, rather than be encryptd along with the plaintext?
> 
> A good understanding of why this is can be had at
> http://blog.thoughtcrime.org/the-cryptographic-doom-principle

Thanks for pointing this out.  I'll change it to keep the HMAC separate and
compute it on the encrypted package.  My reasoning was: if I were going to
tamper with the message and then manufacture a new HMAC that passes, I'd have a
lot harder time inserting it into the message if the HMAC were part of the
encrypted package, but you (and the article) make a good point.

But also read my response below: Are Vaudenay attack concerns even addressable
for open source software?

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
> chrome/browser/chromeos/cros/onc_network_parser.cc:384:
> IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_HMAC_CHECK_FAILED);
> The fact that HMAC errors are distinguished from encryption errors can be used
> to potentially mount a Vaudenay attack. At the least, there is some timing
> information being leaked here about which failed.

This is a valid criticism and I did consider the consequences of this when I was
writing it, but I'd like to point out that the encrypting and the decrypting
code are both open source software.  Any attacker can add their own log messages
or timing instrumentation anywhere they want in about the time it takes to
recompile the software.  I deemed it more important in this case for the user to
know the difference between a corrupted package and a tampered one, since that's
important knowledge.

If you still think this is important, then I'd probably also go ahead and
decrypt the message even if the HMAC fails to obscure the timing information.

[Ryan Sleevi, 2012-01-03 20:15:14]

On 2012/01/03 19:40:12, Greg Spencer (Chromium) wrote:
> On 2011/12/24 07:34:08, Ryan Sleevi wrote:
http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
> > chrome/browser/chromeos/cros/onc_network_parser.cc:384:
> > IDS_NETWORK_CONFIG_ERROR_ENCRYPTED_ONC_HMAC_CHECK_FAILED);
> > The fact that HMAC errors are distinguished from encryption errors can be
used
> > to potentially mount a Vaudenay attack. At the least, there is some timing
> > information being leaked here about which failed.
> 
> This is a valid criticism and I did consider the consequences of this when I
was
> writing it, but I'd like to point out that the encrypting and the decrypting
> code are both open source software.  Any attacker can add their own log
messages
> or timing instrumentation anywhere they want in about the time it takes to
> recompile the software.  I deemed it more important in this case for the user
to
> know the difference between a corrupted package and a tampered one, since
that's
> important knowledge.
> 
> If you still think this is important, then I'd probably also go ahead and
> decrypt the message even if the HMAC fails to obscure the timing information.

So, there's a school of thought that says that you should do exactly that -
perform the same operations even if the failure is known. It depends on the
threat-model you're trying to defend against - which I'm not presently sure of
(the design docs are internal only, AFAICT)

The Vaudenay attack only comes in to play if/when the possibility of an oracle
exists. AFAICT, in it's current implementation, this code is only exercised by
direct result of user interaction. Therefore there is a concrete cost to
launching multiple attempts, such as to effectively discourage probing
(especially of timing).

However, CPA/CCA oracles can be easily and accidentally introduced (such as code
to automatically load these), so I think it's something to still consider - but
/may/ not require direct action yet. Robustness says yes, but
practicality/agility may say "not yet".

If you're trying to defend against rogue processes on the same machine (and this
may even include things like Javascript w/ the high-res web timing drafts being
proposed), then it may be a good (but admittedly not perfect) countermeasure to
perform both operations. 

If you're trying to defend against remote timing attacks, then I think this
would also be a good change, since it's operationally equivalent to RSA blinding
(the classic paper being http://crypto.stanford.edu/~dabo/papers/ssl-timing.pdf
)

My (perhaps naive) hope is that the development/timing costs of implementing the
various defenses is minimal, and in the event an oracle is accidentally
introduced, they will be in place. 

http://netifera.com/research/poet/PaddingOraclesEverywhereEkoparty2010.pdf is
another good example about how even the littlest bit of an oracle can lead to
big breaks.

[Greg Spencer (Chromium), 2012-01-03 21:54:21]

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8949056/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:394: if
(parse_error_.empty())
On 2011/12/22 20:55:17, Charlie Lee wrote:
> when would this not be empty?

When the Deserialize call above places an error into it.

The idea is that if the deserializer doesn't add an error, but we still don't
get a valid root for some reason, then we  need to supply an error.

[Greg Spencer (Chromium), 2012-01-03 22:42:40]

On 2012/01/03 20:15:14, Ryan Sleevi wrote:
> > If you still think this is important, then I'd probably also go ahead and
> > decrypt the message even if the HMAC fails to obscure the timing
information.
> 
> So, there's a school of thought that says that you should do exactly that -
> perform the same operations even if the failure is known. It depends on the
> threat-model you're trying to defend against - which I'm not presently sure of
> (the design docs are internal only, AFAICT)

Yes, the designs are still in draft.  There is an early draft of the doc here:

http://dev.chromium.org/chromium-os/chromiumos-design-docs/open-network-confi...

But the only section that applies is "Encryption", and that's completely changed
in the current draft.
I'll see if we can post a newer draft (I don't own the doc, so I don't want to
do it prematurely).

Note that the encryption we're doing here is a simple PSK methodology: it's far
more likely that these files will be cracked because someone used "password" as
the passphrase than it is that they will be broken by some more esoteric method.
 Even given a good password, the files are meant to be distributed by IT people
to their users, so the transport method of the passphrase is the weak link. 
That said, we still want it to be robust if users take appropriate precautions
to distribute the key.

> The Vaudenay attack only comes in to play if/when the possibility of an oracle
> exists. AFAICT, in it's current implementation, this code is only exercised by
> direct result of user interaction. Therefore there is a concrete cost to
> launching multiple attempts, such as to effectively discourage probing
> (especially of timing).

OK, but the code is open to the public, so there's no way to prevent someone
from recompiling it with some minor changes and performing attempts to break
some input in a tight loop, so while it would be nice to prevent a Vaudenay
attack, I think it's immaterial: an attacker can introduce new code paths on
their local copy that reintroduce the data that would be needed for a Vaudenay
attack to proceed, so there's no point in "preventing" it by not supplying the
data that there's an HMAC failure versus a decryption failure: at best we're
slowing an attacker down by 10 minutes, at worst, we're hiding the fact from a
legitimate user that their package was tampered with instead of just being
corrupted in transit.

> If you're trying to defend against rogue processes on the same machine (and
this
> may even include things like Javascript w/ the high-res web timing drafts
being
> proposed), then it may be a good (but admittedly not perfect) countermeasure
to
> perform both operations.

Hmm.  I'm trying to imagine what an attack like this might be like, but I can't
see how it would be easier to do something tricky in JavaScript than it would be
to try straightforward stuff locally in C++.  (Other than somehow sniffing out
the passphrase directly, which would be an obvious Bad Thing, but not affected
by the HMAC error changes).

> If you're trying to defend against remote timing attacks, then I think this
> would also be a good change, since it's operationally equivalent to RSA
blinding
> (the classic paper being
http://crypto.stanford.edu/%7Edabo/papers/ssl-timing.pdf
> )

Remote timing attacks aren't necessary: they can just compile the code and try
it locally.

> My (perhaps naive) hope is that the development/timing costs of implementing
the
> various defenses is minimal, and in the event an oracle is accidentally
> introduced, they will be in place. 

The cost isn't high at all: I'm happy to implement it if it makes sense, I just
think it hides information that's useful to the user, and doesn't help protect
anything because the code is open anyhow.

[Ryan Sleevi, 2012-01-03 23:07:51]

On 2012/01/03 22:42:40, Greg Spencer (Chromium) wrote:
> Yes, the designs are still in draft.  There is an early draft of the doc here:
> 
>
http://dev.chromium.org/chromium-os/chromiumos-design-docs/open-network-confi...
> 
> But the only section that applies is "Encryption", and that's completely
changed
> in the current draft.
> I'll see if we can post a newer draft (I don't own the doc, so I don't want to
> do it prematurely).
> 
> Note that the encryption we're doing here is a simple PSK methodology: it's
far
> more likely that these files will be cracked because someone used "password"
as
> the passphrase than it is that they will be broken by some more esoteric
method.
>  Even given a good password, the files are meant to be distributed by IT
people
> to their users, so the transport method of the passphrase is the weak link. 
> That said, we still want it to be robust if users take appropriate precautions
> to distribute the key.

Right - I realize that it's using PBKDF2 to derive a key, which means that the
only mitigation against an offline attack brute force is the iteration count.

The suggested changes do nothing to improve the security against this attack -
the only good way is to increase the iteration count and ensure the IV isn't
re-used.

> 
> > The Vaudenay attack only comes in to play if/when the possibility of an
oracle
> > exists. AFAICT, in it's current implementation, this code is only exercised
by
> > direct result of user interaction. Therefore there is a concrete cost to
> > launching multiple attempts, such as to effectively discourage probing
> > (especially of timing).
> 
> OK, but the code is open to the public, so there's no way to prevent someone
> from recompiling it with some minor changes and performing attempts to break
> some input in a tight loop, so while it would be nice to prevent a Vaudenay
> attack, I think it's immaterial: an attacker can introduce new code paths on
> their local copy that reintroduce the data that would be needed for a Vaudenay
> attack to proceed, so there's no point in "preventing" it by not supplying the
> data that there's an HMAC failure versus a decryption failure: at best we're
> slowing an attacker down by 10 minutes, at worst, we're hiding the fact from a
> legitimate user that their package was tampered with instead of just being
> corrupted in transit.

I think we may be at cross-purposes here, which may explain the disconnect.
Open/Closed source doesn't matter much - if you have an oracle (whether you
compile it yourself or it exists as a service), Vaudenay is a risk. It doesn't
really become more of a risk if you can compile your own - if you have an
oracle, you just keep throwing inputs at it.

Encrypt-then-HMAC & Encrypt-and-HMAC are both meant to be mitigations against
the risk of an online attack, where the passphrase doesn't have to be entered.
While I realize in the current implementation, user input is required, to input
the passphrase, it's "possible" that consumers/implementations of this may rely
on the machine being pre-configured with the passphrase, and the provisioning
tool knowing this shared secret. The user would need merely upload an ONC file
and be told "yay" or "nay". Whether this means online (eg: the ONC
parser/validator is launched automatically by say, some particular mime type or
particular protocol) or offline (the user has to manually upload a file), either
way an oracle is created.

The attacker doesn't need to attack the key then - they can use the oracle to
recover the ciphertext, which presumably would include network secrets.

Again, these mitigations are based on the assumption that the "passphrase" part
of the protocol may be automatically handled at some point in the future - and
that the user (attacker) doesn't know the passphrase ergo doesn't know the
settings.

If the user knows the passphrase, then they can always decrypt the file
themselves - there are no other encryption keys, AFAICT, so giving the
passphrase to the user is the same as giving them all the information.

> 
> > If you're trying to defend against rogue processes on the same machine (and
> this
> > may even include things like Javascript w/ the high-res web timing drafts
> being
> > proposed), then it may be a good (but admittedly not perfect) countermeasure
> to
> > perform both operations.
> 
> Hmm.  I'm trying to imagine what an attack like this might be like, but I
can't
> see how it would be easier to do something tricky in JavaScript than it would
be
> to try straightforward stuff locally in C++.  (Other than somehow sniffing out
> the passphrase directly, which would be an obvious Bad Thing, but not affected
> by the HMAC error changes).

Again, operating on the assumption an oracle exists, then you could use various
timings to determine how far along the process the oracle went. If the code
early terminates on a MAC failure, you might see that a JS operation (web page
load, complex script, w/e) takes 50ms w/o any operation, takes 40ms to return
some "nay" responses, but then takes 400ms to return others, you've now got an
oracle to determine HMAC vs Encryption.

But again, it presupposes that the key is already entered (known to the
decoder), but not necessarily known to the user.


Since we're talking about a (hopefully) general purpose format, building in
those mitigations against an oracle provide a secondary defense. The format is
now robust enough that future implementations may use passphrases with high
iteration counts that pre-provision to a particular set of machines (eg: a
machine key/organziation key stored in a PSM that is unknown to users/recepients
of the ONC file).

In the current implementation for ChromiumOS, because the user is required to
know and input the passphrase, these particular attacks are not a risk. But the
format in general can be shored up to defend against them, and then in the event
someone "accidentally" introduces an oracle at some point in the future, or if
one is shown to exist elsewhere, then the class of attacks is defended against.

Paranoia, mostly :)

[Greg Spencer (Chromium), 2012-01-04 00:00:43]

On 2012/01/03 23:07:51, Ryan Sleevi wrote:
> In the current implementation for ChromiumOS, because the user is required to
> know and input the passphrase, these particular attacks are not a risk. But
the
> format in general can be shored up to defend against them, and then in the
event
> someone "accidentally" introduces an oracle at some point in the future, or if
> one is shown to exist elsewhere, then the class of attacks is defended
against.

OK, I think I get your point: it won't make it more secure at the moment, but
you want to make sure that in the future it's still secure if we change the way
it is used.  This makes sense.

So for purposes of this change, would specifying Encrypt-then-HMAC in the format
be sufficient, or would I also need to remove the difference in the error codes
returned to the user (e.g. Failed HMAC vs failed decrypt)?

I do realize that a failed HMAC on encrypted data could simply be corruption and
not tampering: to report tampering to the user more effectively, I'd have to
succeed in decrypting the data even after a failed HMAC (but still fail overall
and report tampering).  But would that result in effectively the same issue as
HMAC-then-Encrypt?

Or am I just silly to want to tell the user this information at all?

[Ryan Sleevi, 2012-01-04 00:12:50]

On 2012/01/04 00:00:43, Greg Spencer (Chromium) wrote:
> So for purposes of this change, would specifying Encrypt-then-HMAC in the
format
> be sufficient, or would I also need to remove the difference in the error
codes
> returned to the user (e.g. Failed HMAC vs failed decrypt)?

Specifying Encrypt-then-HMAC in the spec should be sufficient to ensure this
vector can be defended against. Defense against timing-related attacks or acting
as an oracle can be done in the implementation , without affecting the
correctness/conformance.

> 
> I do realize that a failed HMAC on encrypted data could simply be corruption
and
> not tampering: to report tampering to the user more effectively, I'd have to
> succeed in decrypting the data even after a failed HMAC (but still fail
overall
> and report tampering).  But would that result in effectively the same issue as
> HMAC-then-Encrypt?
> 
> Or am I just silly to want to tell the user this information at all?

As an end-user who receives this message, will I take any different actions
based on whether it was an HMAC error vs encryption error?

As a system administrator, will I take any different action?

I suspect that in both cases, the answer will be to either re-download the file
or (as an admin) re-generate it and send it. So knowing the distinction between
the two errors doesn't really improve the situation.

As you said, detecting tampering vs corruption really is hard - since an
improperly tampered-with message will be indistinguishable from corruption. I
personally don't think there is much value in providing the distinction - and if
it was reported to an attacker (again, "attacker" in the sense that there is an
oracle), it's still information disclosure that can be easily avoided.

I don't believe in the case of Encrypt-then-HMAC that it's strictly necessary to
mask the HMAC-vs-Encrypt errors, but I'd defer to agl@-et-al if there is a class
of attacks I'm not yet familiar with.

[Greg Spencer (Chromium), 2012-01-04 00:22:54]

On 2012/01/04 00:12:50, Ryan Sleevi wrote:
> Specifying Encrypt-then-HMAC in the spec should be sufficient to ensure this
> vector can be defended against. Defense against timing-related attacks or
acting
> as an oracle can be done in the implementation , without affecting the
> correctness/conformance.

OK, done.

> > Or am I just silly to want to tell the user this information at all?
> 
> As an end-user who receives this message, will I take any different actions
> based on whether it was an HMAC error vs encryption error?
> 
> As a system administrator, will I take any different action?

Well, my vision is that the system administrator would then be aware that some
attacker existed and investigate, but I admit that this is unlikely.

I removed the difference in the error messages as well.

[Greg Spencer (Chromium), 2012-01-04 00:53:28]

Adding Adam for review, since this implementation involves encryption/security
issues.

[Ryan Sleevi, 2012-01-04 01:33:43]

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:23: #include
"crypto/encryptor.h"
nit: lexicographical sort

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:319: if
(!base::Base64Decode(hmac, &hmac)) {
nit: Move these decodes (Line 304, 314, 319) above the decryptor init (309) and
HMAC verify (328).

The reasoning (which is admittedly weak) is that .Init() may perform non-trivial
work (eg: a really high iteration) and thus may block for a while.

Related to that above statement, is it safe to assume this code all runs on a
worker thread? If not, what prevents someone from sending a really high
iteration count and blocking a browser thread (assuming FILE || IO)

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:343: if (!new_root.get() ||
new_root->GetType() != base::Value::TYPE_DICTIONARY) {
nit: I believe the preference is to use

new_root->IsType(base::Value::TYPE_DICTIONARY)

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:349: return
static_cast<base::DictionaryValue*>(new_root.release());
side nit: From an API perspective, it's unclear which is more correct - this
call (assuming DictionaryValue) or using the base::Value::GetAsDictionary()
method.

From just reading the headers, it seems like GetAsDictionary is "more" correct,
but perhaps this is just trivium.

http://codereview.chromium.org/8949056/diff/13010/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/8949056/diff/13010/crypto/hmac.cc#newcode17
crypto/hmac.cc:17: key.GetRawKey(&raw_key);
GetRawKey can fail (eg: non-exportable)

bool result = key.GetRawKey(&raw_key) && Init(raw_key);

I suppose a TODO at some point will be to not require exporting it to a raw key
(It really isn't needed if we have a SymmetricKey, from an underlying OS
perspective - so it's solely an API design issue)

http://codereview.chromium.org/8949056/diff/13010/crypto/hmac.cc#newcode22
crypto/hmac.cc:22: raw_key.reserve(0);
Unfortunately, it's possible (and in practice, likely) for this to be optimized
out. Namely, the compiler can realize this is a memset(raw_ptr, 0, size)
followed by a free(raw_ptr), see that raw_ptr is never accessed after the
memset, and optimize out the memset.

So if you're trying to do the equivalent of SecureZeroMemory and wipe the
memory, more is needed. For more info about the ongoing arms race w/ compilers,
see
https://www.securecoding.cert.org/confluence/display/cplusplus/MSC06-CPP.+Be+...

If it's not that important (again, depends on the risk scenario, although I am a
fan of memory scrubbing), it's probably safe to remove.

If you do add it, see crypto/secure_util.h for the ideal place :)

http://codereview.chromium.org/8949056/diff/13010/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/8949056/diff/13010/crypto/symmetric_key.h#newc...
crypto/symmetric_key.h:73: bool GetRawKey(std::string* raw_key) const;
This isn't a const operation on Windows, at least not according to the API
specs. Getting the raw key can mutate the underlying key IIRC.

On Mac it works because we keep a copy of the raw key around, but normally (eg:
CSSM-only) it would be the same as Windows.

[Greg Spencer (Chromium), 2012-01-05 22:18:03]

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:23: #include
"crypto/encryptor.h"
On 2012/01/04 01:33:43, Ryan Sleevi wrote:
> nit: lexicographical sort

Done.

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:319: if
(!base::Base64Decode(hmac, &hmac)) {
On 2012/01/04 01:33:43, Ryan Sleevi wrote:
> nit: Move these decodes (Line 304, 314, 319) above the decryptor init (309)
and
> HMAC verify (328).

Done.

> The reasoning (which is admittedly weak) is that .Init() may perform
non-trivial
> work (eg: a really high iteration) and thus may block for a while.
> 
> Related to that above statement, is it safe to assume this code all runs on a
> worker thread? If not, what prevents someone from sending a really high
> iteration count and blocking a browser thread (assuming FILE || IO)

There's really nothing preventing that at the moment.  When we integrate this
into the final UI, it will need to be run on a worker thread.

Maybe I should add a sanity check for the iteration count (so it has to be, say,
under 50K)?

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:343: if (!new_root.get() ||
new_root->GetType() != base::Value::TYPE_DICTIONARY) {
On 2012/01/04 01:33:43, Ryan Sleevi wrote:
> nit: I believe the preference is to use
> 
> new_root->IsType(base::Value::TYPE_DICTIONARY)

Done.

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:349: return
static_cast<base::DictionaryValue*>(new_root.release());
On 2012/01/04 01:33:43, Ryan Sleevi wrote:
> side nit: From an API perspective, it's unclear which is more correct - this
> call (assuming DictionaryValue) or using the base::Value::GetAsDictionary()
> method.
> 
> From just reading the headers, it seems like GetAsDictionary is "more"
correct,
> but perhaps this is just trivium.

I see your point, but this is what it translates into, since I still need to
release the pointer from the scoped_ptr:

  base::DictionaryValue* result = NULL;
  new_root->GetAsDictionary(&result);
  new_root.release();
  return result;

And that seems a lot more cumbersome, and no safer in the end, since we've
already verified that it really is a dictionary by this point.

http://codereview.chromium.org/8949056/diff/13010/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/8949056/diff/13010/crypto/hmac.cc#newcode22
crypto/hmac.cc:22: raw_key.reserve(0);
On 2012/01/04 01:33:43, Ryan Sleevi wrote:
> Unfortunately, it's possible (and in practice, likely) for this to be
optimized
> out.
> [...]
> If you do add it, see crypto/secure_util.h for the ideal place :)

I took this code from the HMAC destructor on mac, so that will probably need
fixing too.

Is it sufficient to call std::fill(key_.begin(), key_.end(), 0)?  I'm guessing
not, especially in a destructor where the compiler knows you're not going to
touch it again.  In that case, we also have problems in the SymmetricKey
destructor(s).

I started to implement SecureZeroMemory and SecureZeroString, and decided that
although it is probably correct to blank out any key material, simply using
std::string to store the key material is problematic in that regard: there's no
controlling when it will realloc and copy without clearing the old key data
memory.  The only way to do that would be to implement our own string allocator
and create a different basic_string specialization, and I think that's probably
way beyond the scope of this CL.

I'm going to go ahead and switch this to use std::fill as in the SymmetricKey
destructors, and then assign the result of raw_key.c_str() to a volatile char*
in the hopes that this will force the compiler to leave it around, but I'm not
optimistic that it will be sufficient in the long run (it might just touch the
first byte).  When compiling a test program with -O3 (and -g for debug info), it
appears to work in the debugger, but that might not be enough of a test, since
g++ has so many different optimization modes.

I've also noticed several places (mostly in the sync code) were we don't even
zero things out after using the key, so perhaps it's not that critical.

I could still implement the functions if you want, but probably not in this CL.

http://codereview.chromium.org/8949056/diff/13010/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/8949056/diff/13010/crypto/symmetric_key.h#newc...
crypto/symmetric_key.h:73: bool GetRawKey(std::string* raw_key) const;
On 2012/01/04 01:33:43, Ryan Sleevi wrote:
> This isn't a const operation on Windows, at least not according to the API
> specs. Getting the raw key can mutate the underlying key IIRC.
> 
> On Mac it works because we keep a copy of the raw key around, but normally
(eg:
> CSSM-only) it would be the same as Windows.

Crap.  OK, I'll change the constness of the arguments all the way up.. Grrr.

[Ryan Sleevi, 2012-01-05 22:34:10]

I'll leave Adam and Charlie to give the final LGs, but all of my concerns appear
to have been addressed.

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:319: if
(!base::Base64Decode(hmac, &hmac)) {
On 2012/01/05 22:18:03, Greg Spencer (Chromium) wrote:
> There's really nothing preventing that at the moment.  When we integrate this
> into the final UI, it will need to be run on a worker thread.
> 
> Maybe I should add a sanity check for the iteration count (so it has to be,
say,
> under 50K)?

For in-progress code, I think it's fine as-is, as long as you're aware of the
risk and plan to address it. If you think it might get dropped, perhaps add a
TODO here and file a tracking bug for it, just so that we can revisit?

A worker thread + a timer will be a much more robust solution than a fixed upper
limit, since as processors improve (and algorithms are weakened), it's natural
to see the iteration count go up. Classic example is iPhone going from 4K
iterations -> 10K -> 40K presently, IIRC.

I think 50K may be a bit too low (even if it's probably a reasonable number).
Setting the upper limit to something closer to 100-150K is probably good enough
for the paranoid 'for now' (and for the +6 month timeframe that it would take to
deploy an update if the max needed to be bumped)

But for this CL, as long as you're aware of it, I don't think anything else
codewise needs to be done.

http://codereview.chromium.org/8949056/diff/13010/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:349: return
static_cast<base::DictionaryValue*>(new_root.release());
On 2012/01/05 22:18:03, Greg Spencer (Chromium) wrote:
> On 2012/01/04 01:33:43, Ryan Sleevi wrote:
> > side nit: From an API perspective, it's unclear which is more correct - this
> > call (assuming DictionaryValue) or using the base::Value::GetAsDictionary()
> > method.
> > 
> > From just reading the headers, it seems like GetAsDictionary is "more"
> correct,
> > but perhaps this is just trivium.
> 
> I see your point, but this is what it translates into, since I still need to
> release the pointer from the scoped_ptr:
> 
>   base::DictionaryValue* result = NULL;
>   new_root->GetAsDictionary(&result);
>   new_root.release();
>   return result;
> 
> And that seems a lot more cumbersome, and no safer in the end, since we've
> already verified that it really is a dictionary by this point.

Agreed - but it does seem like the API (of the base::Value) is a bit "broken" in
that it provides a convenience method (a /virtual/ one, on top of that, so
clearly meant to accommodate class hierarchies) that is less useful/more
cumbersome for users.

It's a side nit, and leaving it as is seems to make sense.

http://codereview.chromium.org/8949056/diff/13010/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/8949056/diff/13010/crypto/hmac.cc#newcode22
crypto/hmac.cc:22: raw_key.reserve(0);
On 2012/01/05 22:18:03, Greg Spencer (Chromium) wrote:
> On 2012/01/04 01:33:43, Ryan Sleevi wrote:
> > Unfortunately, it's possible (and in practice, likely) for this to be
> optimized
> > out.
> > [...]
> > If you do add it, see crypto/secure_util.h for the ideal place :)
> 
> I took this code from the HMAC destructor on mac, so that will probably need
> fixing too.
> 
> Is it sufficient to call std::fill(key_.begin(), key_.end(), 0)?  I'm guessing
> not, especially in a destructor where the compiler knows you're not going to
> touch it again.  In that case, we also have problems in the SymmetricKey
> destructor(s).
> 
> I started to implement SecureZeroMemory and SecureZeroString, and decided that
> although it is probably correct to blank out any key material, simply using
> std::string to store the key material is problematic in that regard: there's
no
> controlling when it will realloc and copy without clearing the old key data
> memory.  The only way to do that would be to implement our own string
allocator
> and create a different basic_string specialization, and I think that's
probably
> way beyond the scope of this CL.
> 
> I'm going to go ahead and switch this to use std::fill as in the SymmetricKey
> destructors, and then assign the result of raw_key.c_str() to a volatile char*
> in the hopes that this will force the compiler to leave it around, but I'm not
> optimistic that it will be sufficient in the long run (it might just touch the
> first byte).  When compiling a test program with -O3 (and -g for debug info),
it
> appears to work in the debugger, but that might not be enough of a test, since
> g++ has so many different optimization modes.
> 
> I've also noticed several places (mostly in the sync code) were we don't even
> zero things out after using the key, so perhaps it's not that critical.
> 
> I could still implement the functions if you want, but probably not in this
CL.

std::fill() just gets turned into a memset(), due to the iterator traits of
.begin()/.end() (pointer type). Which then goes back to the original problem.

I agree absolutely that using std::string() to secure secrets is problematic, IF
operating under a threat-model that keeping secrets in RAM may cause them to be
leak. I'm not aware of us in general really considering that threat model as
valid - otherwise we'd also look at things like
CryptProtectData/CryptUnprotectData for tranient work.

I agree it's beyond the scope of this CL - which is where I was trying to go
with it. Under an optimization pass, this can ("should") be optimized out, so it
doesn't really buy much from security; thus, it could be removed. If it is a
concern/part of the threat model, we should file a bug and start auditing code.
But like I said above, I don't really think this has been a threat-model we've
entertained.

[kmixter1, 2012-01-05 23:27:53]

http://codereview.chromium.org/8949056/diff/20001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8949056/diff/20001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:1771: virtual bool
LoadOncNetworks(const std::string& onc_blob,
Where is the code that calls this with the new parameter?

http://codereview.chromium.org/8949056/diff/20001/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/8949056/diff/20001/crypto/hmac.cc#newcode18
crypto/hmac.cc:18: // Zero out key copy.  This probably just gets optimized
away,
I don't think the stuff after fill will have any effect.  If you really want to
be sure that the clearing is not optimized away, instead do this:

volatile char* heap_pointer = raw_key.data();
memset(heap_pointer, 0, raw_key.size());
return result;

Then you've created an aliasing pointer to volatile memory whose accesses by
memset could never be optimized away.

That's all theoretical.  I don't think a compiler exists that does enough heap
analysis to remove write heap accesses before free's, even in C much less
through all the spaghetti of C++ stl functions. :)

[Greg Spencer (Chromium), 2012-01-05 23:41:05]

http://codereview.chromium.org/8949056/diff/20001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8949056/diff/20001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:1771: virtual bool
LoadOncNetworks(const std::string& onc_blob,
On 2012/01/05 23:27:53, kmixter1 wrote:
> Where is the code that calls this with the new parameter?

chrome/browser/ui/webui/net_internals_ui.cc:1302

It's not part of this CL because it was already there from a previous UI change.

http://codereview.chromium.org/8949056/diff/20001/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/8949056/diff/20001/crypto/hmac.cc#newcode18
crypto/hmac.cc:18: // Zero out key copy.  This probably just gets optimized
away,
On 2012/01/05 23:27:53, kmixter1 wrote:
> I don't think the stuff after fill will have any effect.  If you really want
to
> be sure that the clearing is not optimized away, instead do this:
> 
> volatile char* heap_pointer = raw_key.data();
> memset(heap_pointer, 0, raw_key.size());
> return result;
> 
> Then you've created an aliasing pointer to volatile memory whose accesses by
> memset could never be optimized away.
> 
> That's all theoretical.  I don't think a compiler exists that does enough heap
> analysis to remove write heap accesses before free's, even in C much less
> through all the spaghetti of C++ stl functions. :)

OK, I'll do that.  Why is mine different (other than order)?  Seems like the
compiler couldn't assume that the raw_key was not accessed if we access it and
assign to a volatile.  Is it the use of c_str() instead of data()?

[agl, 2012-01-06 15:59:56]

LGTM

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:260: const int
key_size_in_bits = 256;
(ignore if you like.) I'd expect a constant to be called kKeySizeInBits, or even
kKeyBits.

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:304: iterations,
I know that Ryan has already commented on this, but I really would put in a
sanity check on iterations. Even if it's just checking that it's less than a
million.

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:306: 
(ignore if you like.) Extra blank line?

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:327: if
(!decryptor.Init(key.get(), crypto::Encryptor::CBC, initial_vector))  {
(ignore if you like.) Move decryptor.Init below the HMAC check so that it's just
above decryptor.Decrypt?

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:332: // Verify the HMAC.  If
it fails, then we will still try to decrypt because
I don't think that you need to worry about the timing here. Timing is an issue
when the encryption wraps the MAC because a failed decryption leaks information
about the validity of the padding.

However, we can assume that the HMAC works - i.e. that the attacker cannot forge
a valid HMAC. At that point, invalid padding can only be caused by a broken
implementation at the generator.

[kmixter1, 2012-01-06 17:55:58]

http://codereview.chromium.org/8949056/diff/20001/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/8949056/diff/20001/crypto/hmac.cc#newcode18
crypto/hmac.cc:18: // Zero out key copy.  This probably just gets optimized
away,
On 2012/01/05 23:41:06, Greg Spencer (Chromium) wrote:
> On 2012/01/05 23:27:53, kmixter1 wrote:
> > I don't think the stuff after fill will have any effect.  If you really want
> to
> > be sure that the clearing is not optimized away, instead do this:
> > 
> > volatile char* heap_pointer = raw_key.data();
> > memset(heap_pointer, 0, raw_key.size());
> > return result;
> > 
> > Then you've created an aliasing pointer to volatile memory whose accesses by
> > memset could never be optimized away.
> > 
> > That's all theoretical.  I don't think a compiler exists that does enough
heap
> > analysis to remove write heap accesses before free's, even in C much less
> > through all the spaghetti of C++ stl functions. :)
> 
> OK, I'll do that.  Why is mine different (other than order)?  Seems like the
> compiler couldn't assume that the raw_key was not accessed if we access it and
> assign to a volatile.  Is it the use of c_str() instead of data()?

I was trying to use data() to get at the actual internal representation of the
string, but unfortunately the standard doesn't guarantee it is (and c_str()
definitely isn't as some implementations don't NULL terminate the string
internally).  Using data() as I did is wrong though as the spec says that you
must not write to the pointer returned from it.

So anyway, I don't think the existing code accomplishes avoiding an
optimization, but compilers don't do this so it's probably not worth the thought
exercise of how to do it.

[kmixter1, 2012-01-06 17:56:30]

LGTM

[Greg Spencer (Chromium), 2012-01-06 18:00:28]

On 2012/01/06 17:55:58, kmixter1 wrote:
> I was trying to use data() to get at the actual internal representation of the
> string, but unfortunately the standard doesn't guarantee it is (and c_str()
> definitely isn't as some implementations don't NULL terminate the string
> internally).  Using data() as I did is wrong though as the spec says that you
> must not write to the pointer returned from it.

Well, but we don't need to write to it, we just need to access it.

> So anyway, I don't think the existing code accomplishes avoiding an
> optimization, but compilers don't do this so it's probably not worth the
thought
> exercise of how to do it.

OK, I'll just remove it then, although I find the thought experiment
interesting. :-)

[Greg Spencer (Chromium), 2012-01-06 21:45:55]

Thanks for the reviews!

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:260: const int
key_size_in_bits = 256;
On 2012/01/06 15:59:56, agl wrote:
> (ignore if you like.) I'd expect a constant to be called kKeySizeInBits, or
even
> kKeyBits.

Done.

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:304: iterations,
On 2012/01/06 15:59:56, agl wrote:
> I know that Ryan has already commented on this, but I really would put in a
> sanity check on iterations. Even if it's just checking that it's less than a
> million.

I added a check for values > 500,000.
That should be OK for a number of years, at least, and that will occupy a
current machine for about 20 seconds.

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:306: 
On 2012/01/06 15:59:56, agl wrote:
> (ignore if you like.) Extra blank line?

Removed.

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:327: if
(!decryptor.Init(key.get(), crypto::Encryptor::CBC, initial_vector))  {
On 2012/01/06 15:59:56, agl wrote:
> (ignore if you like.) Move decryptor.Init below the HMAC check so that it's
just
> above decryptor.Decrypt?

Done.

http://codereview.chromium.org/8949056/diff/23002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:332: // Verify the HMAC.  If
it fails, then we will still try to decrypt because
On 2012/01/06 15:59:56, agl wrote:
> I don't think that you need to worry about the timing here. Timing is an issue
> when the encryption wraps the MAC because a failed decryption leaks
information
> about the validity of the padding.
> 
> However, we can assume that the HMAC works - i.e. that the attacker cannot
forge
> a valid HMAC. At that point, invalid padding can only be caused by a broken
> implementation at the generator.

OK, I'll fail earlier on a bad HMAC then.

[commit-bot: I haz the power, 2012-01-06 21:46:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/8949056/31002

[commit-bot: I haz the power, 2012-01-06 21:46:38]

Presubmit check for 8949056-31002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/policy/configuration_policy_handler_chromeos.cc

Presubmit checks took 2.0s to calculate.

[Greg Spencer (Chromium), 2012-01-06 21:48:14]

Mattias,  I need an OWNERS review for
chrome/browser/policy/configuration_policy_handler_chromeos.cc

It's a really simple change...

[Greg Spencer (Chromium), 2012-01-11 17:11:21]

Ping, and adding the other two policy owners.

Can one of you please review this one line change?

[Greg Spencer (Chromium), 2012-01-11 17:11:22]

Ping, and adding the other two policy owners.

Can one of you please review this one line change?

[Greg Spencer (Chromium), 2012-01-11 17:12:38]

(now actually adding the other two policy owners.)

Can one of you please review this one line change?

[Greg Spencer (Chromium), 2012-01-11 17:13:56]

Sorry for the repeated requests: that was rietveld, not me.

On Wed, Jan 11, 2012 at 9:12 AM, <gspencer@chromium.org> wrote:

> (now actually adding the other two policy owners.)
>
>
> Can one of you please review this one line change?
>
>
http://codereview.chromium.**org/8949056/<http://codereview.chromium.org/8949...
>

[Greg Spencer (Chromium), 2012-01-11 18:57:58]

Looks like Joao is the only policy OWNER that isn't on vacation.

Per Zel, I'm going to TBR this (the change that requires OWNERS approval is
trivial).

[commit-bot: I haz the power, 2012-01-11 18:58:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/8949056/30006

[commit-bot: I haz the power, 2012-01-11 18:58:35]

Can't apply patch for file
chrome/browser/chromeos/cros/onc_network_parser_unittest.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
Hunk #1 succeeded at 469 (offset 210 lines).
Hunk #2 succeeded at 487 (offset 210 lines).
Hunk #3 succeeded at 544 (offset 210 lines).
Hunk #4 succeeded at 583 (offset 210 lines).
Hunk #5 succeeded at 622 (offset 210 lines).
Hunk #6 succeeded at 639 (offset 210 lines).
Hunk #7 succeeded at 718 (offset 210 lines).
Hunk #8 FAILED at 574.
Hunk #9 succeeded at 855 (offset 211 lines).
Hunk #10 succeeded at 938 with fuzz 2 (offset 263 lines).
Hunk #11 FAILED at 708.
2 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/cros/onc_network_parser_unittest.cc.rej

[commit-bot: I haz the power, 2012-01-11 22:03:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/8949056/35002

[commit-bot: I haz the power, 2012-01-11 23:17:16]

Change committed as 117321

[Joao da Silva, 2012-01-12 17:47:20]

policy/ changes lgtm