Add base::UnguessableToken

base/nonce_token.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/nonce_token.h"
+
+#include "base/format_macros.h"
+#include "base/rand_util.h"
+#include "base/strings/stringprintf.h"
+
+namespace base {
+
+// If base::NonceToken is no longer 128 bits, the IPC serialization logic and
+// Mojo StructTraits should be updated to match the size of the struct.
+static_assert(sizeof(NonceToken) == 2 * sizeof(uint64_t),

[danakj, 2016/09/15 23:35:15]

Can you put this assert at the site(s) of the code that would need to be updated
instead?

[tguilbert, 2016/09/16 01:03:01]

Done.

+              "base::NonceToken should be of size 2 * sizeof(uint64_t).");
+
+NonceToken::NonceToken(uint64_t high, uint64_t low) : high_(high), low_(low) {}
+
+std::string NonceToken::ToString() const {
+  return base::StringPrintf("(%" PRIu64 ":%" PRIu64 ")", high_, low_);
+}
+
+// static
+NonceToken NonceToken::Create() {
+  NonceToken token;
+  // Use base::RandBytes instead of crypto::RandBytes, because crypto calls the
+  // base version directly, and to prevent the dependency from base/ to crypto/.
+  base::RandBytes(&token, sizeof(token));
+  return token;
+}
+
+void NonceToken::Serialize(uint64_t* high_out, uint64_t* low_out) const {
+  // Serializing an uninitialized NonceToken is a security issue.
+  CHECK(!is_empty());

[danakj, 2016/09/15 23:35:16]

Why not DCHECK?

[tguilbert, 2016/09/16 01:03:01]

Empty NonceTokens can be copied and assigned normally within a process. If
Serialize is called, it is only for the IPC/Mojo/AIDL case. If a process tries
to serialize an empty NonceToken, it means the resource that is meant to be
protected by the unguessability of the NonceToken is not actually being
protected. I think this is a security issue, and that CHECK should therefore be
used here.

WDYT?

[danakj, 2016/09/16 01:21:55]

You're right it's a security issue but you can't trust an untrusted process to
do anything right anyway, so I think using CHECK here is misleading. The
validation should happen at the time of deserialization, which happens in a
trusted place.

[tguilbert, 2016/09/16 22:16:40]

Ah, I understand the point you are making.

My concern however was more along the lines of the Browser --> Renderer path.
If, due to some edge case, some ID that is meant to be unguessable is never
initialized in the Browser process, then the serialization is the only place
where we can perform validations in a trusted place. If an empty ID is
mistakenly sent by the browser to a valid Renderer, I feel like this will be
hard to debug without a stack trace that shows the browser is the culprit
(because, any valid Renderer that deserializes the 0 token that the browser sent
would then be killed, from what I understand).

That being said, it is a DCHECK now.

+  *high_out = high_;
+  *low_out = low_;
+}
+
+// static
+NonceToken NonceToken::Deserialize(uint64_t high, uint64_t low) {
+  // Sending a zeroed out NonceToken across processes means that it was never
+  // initialized via NonceToken::Create(), which is a security issue.
+  CHECK(!NonceToken::IsZeroData(high, low));

[danakj, 2016/09/15 23:35:16]

I don't think CHECK is appropriate here, it means a misbehaving renderer can
crash the whole browser. When deserializing, if the message is invalid we should
just fail to deserialize it and destroy the misbehaving process.

[tguilbert, 2016/09/16 01:03:01]

The IPC and the Mojo deserialization code use NonceToken::IsZeroData() to make
sure the given bits are not equal to zero before calling deserialize. If
high/low are both equal to 0 in the IPC and the Mojo case,
StructTraits::Read()/ParamTraits::Read() return false, and never calls
Deserialize.

In Mojo's case, this prompts the browser process to kill the misbehaving
renderer.(https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/_Dn9YV9ghxs/S2tZ0mnwBAAJ)

In IPC's case, I do not have a clear cut comment to link to, (but I assume it is
the same).
@tsepez: Is returning false from ParamTraits::Read() sufficient?


I added a comment to Deserialize() in the .h saying that IsZeroData() should be
used to validate the data in cases where we can gracefully fail the
deserialization.

[danakj, 2016/09/16 01:21:55]

That sounds good, but then I don't think you need this CHECK. You can DCHECK to
verify that this does indeed not happen, but please don't overuse CHECK.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

+  return NonceToken(high, low);
+}
+
+}  // namespace base