Upstream: Build net_unittests for Android.

net/base/x509_certificate_openssl.cc

 // Copyright (c) 2011 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 "net/base/x509_certificate.h"
 
 #include <openssl/asn1.h>
 #include <openssl/crypto.h>
 #include <openssl/obj_mac.h>
 #include <openssl/pem.h>
 #include <openssl/pkcs7.h>
 #include <openssl/sha.h>
 #include <openssl/ssl.h>
 #include <openssl/x509v3.h>
 
 #include "base/memory/singleton.h"
 #include "base/pickle.h"
 #include "base/sha1.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "crypto/openssl_util.h"
 #include "net/base/asn1_util.h"
 #include "net/base/cert_status_flags.h"
 #include "net/base/cert_verify_result.h"
 #include "net/base/net_errors.h"
 #include "net/base/x509_util_openssl.h"
 
+#if defined(OS_ANDROID)
+#include "base/logging.h"
+#include "net/android/network_library.h"
+#endif
+
 namespace net {
 
 namespace {
 
 void CreateOSCertHandlesFromPKCS7Bytes(
     const char* data, int length,
     X509Certificate::OSCertHandles* handles) {
   crypto::EnsureOpenSSLInit();
   const unsigned char* der_data = reinterpret_cast<const unsigned char*>(data);
   crypto::ScopedOpenSSL<PKCS7, PKCS7_free> pkcs7_cert(
@@ skipping 467 matching lines @@
     ip_addrs->clear();
 
   ParseSubjectAltName(cert_handle_, dns_names, ip_addrs);
 }
 
 // static
 X509_STORE* X509Certificate::cert_store() {
   return X509InitSingleton::GetInstance()->store();
 }
 
-#if !defined(OS_ANDROID)
-
+#if defined(OS_ANDROID)
 int X509Certificate::VerifyInternal(const std::string& hostname,
                                     int flags,
                                     CRLSet* crl_set,
+                                    CertVerifyResult* verify_result) const {
+  if (!VerifyNameMatch(hostname))
+    verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
+
+  std::vector<std::string> cert_bytes;
+  GetChainDEREncodedBytes(&cert_bytes);
+
+  // TODO(joth): Fetch the authentication type from SSL rather than hardcode.
+  // TODO(jingzhao): Recover the original implementation once we support JNI.
+#if 0
+  android::VerifyResult result =
+      android::VerifyX509CertChain(cert_bytes, hostname, "RSA");
+#else
+  android::VerifyResult result = android::VERIFY_INVOCATION_ERROR;
+  NOTIMPLEMENTED();
+#endif
+  switch (result) {
+    case android::VERIFY_OK:
+      return OK;

[Ryan Sleevi, 2011/11/06 02:39:48]

random drive by:

What does it mean if android::VerifyX509CertChain returns VERIFY_OK, but the
VerifyNameMatch (on line 525/526) fails? Right now, that error would be
suppressed.

As/if/when Chrom[e/ium] implements more stringent checks, this could potentially
be an issue.

One lesser note (perhaps a TODO, or perhaps never for the android impl.) is to
have the android::VerifyX509CertChain return the constructed certification path.
The reason for this is purely presentation (being able to display the
certificate chain).

It'd be a low priority, as the display code is not yet wired up, but some of the
unit tests are testing that unrelated, untrusted intermediates are pruned from
the certification path. X509Certificate::Verify() has a default implementation
setting the verified_chain = this, but the expectation is that VerifyInternal()
will update the chain as/when appropriate.

[Jing Zhao, 2011/11/07 03:42:49]

I think Joth is the original author of this function. Joth, what do you think
about Ryan's comments (including the other two comments below)?

[joth, 2011/11/07 10:42:20]

oh yes, good spot.
To be consistent with the !ANDROID version below, we should just break here, and
then at the end of the function:-
  if (IsCertStatusError(verify_result->cert_status))
    return MapCertStatusToNetError(verify_result->cert_status);

  return OK;
}
I've got a task to do that as a follow up (http://b/4290005 for the record, but
afraid that tracker is not public)
And I think we'll need it for GetCertChainInfo and AppendPublicKeyHashes too.
And set is_issued_by_known_root....
I think this thread shows we need some continuous builders and  tests running
first though 

[jingzhao, 2011/11/08 02:02:17]

Fixed as you said.
Yes, thank you for the record.

+    case android::VERIFY_BAD_HOSTNAME:
+      verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
+      break;
+    case android::VERIFY_NO_TRUSTED_ROOT:
+      verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID;
+      break;
+    case android::VERIFY_INVOCATION_ERROR:
+    default:
+      verify_result->cert_status |= ERR_CERT_INVALID;
+      break;
+  }
+  return MapCertStatusToNetError(verify_result->cert_status);
+}
+
+#else
+int X509Certificate::VerifyInternal(const std::string& hostname,
+                                    int flags,
+                                    CRLSet* crl_set,
                                     CertVerifyResult* verify_result) const {
   if (!VerifyNameMatch(hostname))
     verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
 
   crypto::ScopedOpenSSL<X509_STORE_CTX, X509_STORE_CTX_free> ctx(
       X509_STORE_CTX_new());
 
   crypto::ScopedOpenSSL<STACK_OF(X509), sk_X509_free_fn> intermediates(
       sk_X509_new_null());
   if (!intermediates.get())
@@ skipping 28 matching lines @@
   // Currently we only ues OpenSSL's default root CA paths, so treat all
   // correctly verified certs as being from a known root. TODO(joth): if the
   // motivations described in http://src.chromium.org/viewvc/chrome?view=rev&revision=80778
   // become an issue on OpenSSL builds, we will need to embed a hardcoded list
   // of well known root CAs, as per the _mac and _win versions.
   verify_result->is_issued_by_known_root = true;
 
   return OK;
 }
 
-#endif  // !defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID)
 
 // static
 bool X509Certificate::GetDEREncoded(X509Certificate::OSCertHandle cert_handle,
                                     std::string* encoded) {
   DERCache der_cache;
   if (!GetDERAndCacheIfNeeded(cert_handle, &der_cache))
     return false;
   encoded->assign(reinterpret_cast<const char*>(der_cache.data),
                   der_cache.data_length);
   return true;
@@ skipping 34 matching lines @@
                                                 Pickle* pickle) {
   DERCache der_cache;
   if (!GetDERAndCacheIfNeeded(cert_handle, &der_cache))
     return false;
 
   return pickle->WriteData(
       reinterpret_cast<const char*>(der_cache.data),
       der_cache.data_length);
 }
 
+#if defined(OS_ANDROID)
+void X509Certificate::GetChainDEREncodedBytes(
+    std::vector<std::string>* chain_bytes) const {
+  OSCertHandles cert_handles(intermediate_ca_certs_);

[Ryan Sleevi, 2011/11/06 02:39:48]

Two things:

1) Why not implement this in terms of GetDEREncoded (line 612), to avoid
diverging DER-accessing implementations? Since you end up copying
|der_cache.data| to a string anyways, there is no extra copy overhead here.

[joth, 2011/11/07 10:42:20]

I think it's just GetDEREncoded didn't exist when the first version of this was
written. I'll do a follow up to refactor this, to avoid iterating it more in
this CL

[jingzhao, 2011/11/08 02:02:17]

This change seems small so I made it.. Please take a look if it's what you want.

[joth, 2011/11/08 10:05:23]

Thanks, this looks OK.

For my own record, when we have test coverage running for this, I would like to
go further:

void X509Certificate::GetChainDEREncodedBytes(..)
  chain_bytes->resize(intermediate_ca_certs_.size() + 1);
  if (!GetDEREncoded(cert_handle_, &chain_bytes->at(0)))
    return <ERROR>
  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
    GetDEREncoded(intermediate_ca_certs_[i], &chain_bytes->at(i + 1));
  }
}

Also the whole function could be inlined at the call-site, avoiding the need for
this extra #ifdef in the .h and .cc files.

[Ryan Sleevi, 2011/11/08 15:03:23]

Even if not unlined to the callsite, you can move this to the anonymous
namespace since GetDEREncoded is public.

If you do further tweak this, just use &cert_handles[i], not
cert_handles->at(i). I get dinged for using it in style reviews too ;-)

+  // Make sure the peer's own cert is the first in the chain, if it's not
+  // already there.
+  if (cert_handles.empty())
+    cert_handles.insert(cert_handles.begin(), cert_handle_);

[Ryan Sleevi, 2011/11/06 02:39:48]

2) This isn't correct how X509Certificate stores its chain, so I'm surprised
this works.

|intermediate_ca_certs_| contains only the intermediates supplied by the server
- it never contains the end-entity cert (|cert_handle_|). For servers who supply
a certificate chain > 1 (ie: most?), this would fail to consider the EE
certificate.

[joth, 2011/11/07 10:42:20]

Good spot, thank you. This was broken when it was landed in
http://codereview.chromium.org/7538029 (which I never saw to review) (which is
also where it was moved to the _android.cc file where it couldn't compile...)

In my version it said:
  if (cert_handles.empty() || cert_handles[0] != cert_handle_)
    cert_handles.insert(cert_handles.begin(), cert_handle_);

As you say intermediate_ca_certs_ never contains cert_handle_ so this guard is
redundant, but it does do the right thing at least.

[jingzhao, 2011/11/08 02:02:17]

Fixed as you said.

+
+  chain_bytes->reserve(cert_handles.size());
+  for (OSCertHandles::const_iterator it = cert_handles.begin();
+       it != cert_handles.end(); ++it) {
+    DERCache der_cache = {0};
+    GetDERAndCacheIfNeeded(*it, &der_cache);
+    std::string cert_bytes (
+        reinterpret_cast<const char*>(der_cache.data), der_cache.data_length);
+    chain_bytes->push_back(cert_bytes);
+  }
+}
+#endif
+
 }  // namespace net