From 047ec2c605410a82d95e1ba677d0b364654c858f Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Tue, 6 Dec 2022 17:01:03 -0800 Subject: [PATCH 1/4] acvptool: factor out uploadResult Change-Id: I7fdc63786654f488b2502d6e9c3fb535a2766574 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55605 Reviewed-by: Adam Langley Reviewed-by: David Benjamin Commit-Queue: Adam Langley --- util/fipstools/acvp/acvptool/acvp.go | 99 +++++++++++++++------------- 1 file changed, 52 insertions(+), 47 deletions(-) diff --git a/util/fipstools/acvp/acvptool/acvp.go b/util/fipstools/acvp/acvptool/acvp.go index 723d29e0b..3e0ad44af 100644 --- a/util/fipstools/acvp/acvptool/acvp.go +++ b/util/fipstools/acvp/acvptool/acvp.go @@ -313,6 +313,54 @@ func getVectorsWithRetry(server *acvp.Server, url string) (out acvp.Vectors, vec } } +func uploadResult(server *acvp.Server, setURL string, resultData []byte) error { + resultSize := uint64(len(resultData)) + 32 /* for framing overhead */ + if server.SizeLimit == 0 || resultSize < server.SizeLimit { + log.Printf("Result size %d bytes", resultSize) + return server.Post(nil, trimLeadingSlash(setURL)+"/results", resultData) + } + + // The NIST ACVP server no longer requires the large-upload process, + // suggesting that this may no longer be needed. + log.Printf("Result is %d bytes, too much given server limit of %d bytes. Using large-upload process.", resultSize, server.SizeLimit) + largeRequestBytes, err := json.Marshal(acvp.LargeUploadRequest{ + Size: resultSize, + URL: setURL, + }) + if err != nil { + return errors.New("failed to marshal large-upload request: " + err.Error()) + } + + var largeResponse acvp.LargeUploadResponse + if err := server.Post(&largeResponse, "/large", largeRequestBytes); err != nil { + return errors.New("failed to request large-upload endpoint: " + err.Error()) + } + + log.Printf("Directed to large-upload endpoint at %q", largeResponse.URL) + req, err := http.NewRequest("POST", largeResponse.URL, bytes.NewBuffer(resultData)) + if err != nil { + return errors.New("failed to create POST request: " + err.Error()) + } + token := largeResponse.AccessToken + if len(token) == 0 { + token = server.AccessToken + } + req.Header.Add("Authorization", "Bearer "+token) + req.Header.Add("Content-Type", "application/json") + + client := &http.Client{} + resp, err := client.Do(req) + if err != nil { + return errors.New("failed writing large upload: " + err.Error()) + } + resp.Body.Close() + if resp.StatusCode != 200 { + return fmt.Errorf("large upload resulted in status code %d", resp.StatusCode) + } + + return nil +} + func main() { flag.Parse() @@ -613,53 +661,10 @@ func main() { resultBuf.Write(replyBytes) resultBuf.WriteString("}") - resultData := resultBuf.Bytes() - resultSize := uint64(len(resultData)) + 32 /* for framing overhead */ - if server.SizeLimit > 0 && resultSize >= server.SizeLimit { - // The NIST ACVP server no longer requires the large-upload process, - // suggesting that it may no longer be needed. - log.Printf("Result is %d bytes, too much given server limit of %d bytes. Using large-upload process.", resultSize, server.SizeLimit) - largeRequestBytes, err := json.Marshal(acvp.LargeUploadRequest{ - Size: resultSize, - URL: setURL, - }) - if err != nil { - log.Printf("Failed to marshal large-upload request: %s", err) - log.Printf("Deleting test set") - server.Delete(url) - os.Exit(1) - } - - var largeResponse acvp.LargeUploadResponse - if err := server.Post(&largeResponse, "/large", largeRequestBytes); err != nil { - log.Fatalf("Failed to request large-upload endpoint: %s", err) - } - - log.Printf("Directed to large-upload endpoint at %q", largeResponse.URL) - client := &http.Client{} - req, err := http.NewRequest("POST", largeResponse.URL, bytes.NewBuffer(resultData)) - if err != nil { - log.Fatalf("Failed to create POST request: %s", err) - } - token := largeResponse.AccessToken - if len(token) == 0 { - token = server.AccessToken - } - req.Header.Add("Authorization", "Bearer "+token) - req.Header.Add("Content-Type", "application/json") - resp, err := client.Do(req) - if err != nil { - log.Fatalf("Failed writing large upload: %s", err) - } - resp.Body.Close() - if resp.StatusCode != 200 { - log.Fatalf("Large upload resulted in status code %d", resp.StatusCode) - } - } else { - log.Printf("Result size %d bytes", resultSize) - if err := server.Post(nil, trimLeadingSlash(setURL)+"/results", resultData); err != nil { - log.Fatalf("Failed to upload results: %s\n", err) - } + if err := uploadResult(server, setURL, resultBuf.Bytes()); err != nil { + log.Printf("Deleting test set") + server.Delete(url) + log.Fatal(err) } } From 23b1ec016404607bd5e867cf5d6fc199294d09d6 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 2 Dec 2022 23:20:00 -0500 Subject: [PATCH 2/4] Fix some more implicit size_t truncations. The buffer length is int, so the output also fits in int. Bug: 516 Change-Id: I8e59a2109f38c81ac58f1a8f1e7d739c8b0d1c7c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55707 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: Bob Beck --- crypto/bio/connect.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/bio/connect.c b/crypto/bio/connect.c index b95fb6bcb..c19f1c4e6 100644 --- a/crypto/bio/connect.c +++ b/crypto/bio/connect.c @@ -363,7 +363,7 @@ static int conn_read(BIO *bio, char *out, int out_len) { } bio_clear_socket_error(); - ret = recv(bio->num, out, out_len, 0); + ret = (int)recv(bio->num, out, out_len, 0); BIO_clear_retry_flags(bio); if (ret <= 0) { if (bio_fd_should_retry(ret)) { @@ -387,7 +387,7 @@ static int conn_write(BIO *bio, const char *in, int in_len) { } bio_clear_socket_error(); - ret = send(bio->num, in, in_len, 0); + ret = (int)send(bio->num, in, in_len, 0); BIO_clear_retry_flags(bio); if (ret <= 0) { if (bio_fd_should_retry(ret)) { From de5bb39125a7758a71274e81bfb1ce68f0bd2148 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 23 Nov 2022 16:32:07 -0500 Subject: [PATCH 3/4] Rename and tidy up x509v3_name_cmp. First, rename to x509v3_conf_name_matches and flip the result value. We don't need to preserve the positive vs negative return of strncmp here. The rename is because "name" can mean so many things in the context of X.509. Here, it's specifically the name of a CONF_VALUE. Finally, fix it to be size_t-clean. Bug: 516 Change-Id: I1c3039d9c6ce70cde669e07f943ad1e25fb49dc1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55705 Commit-Queue: Bob Beck Reviewed-by: Bob Beck Auto-Submit: David Benjamin --- crypto/x509v3/internal.h | 6 +++--- crypto/x509v3/v3_alt.c | 20 ++++++++++---------- crypto/x509v3/v3_cpols.c | 4 ++-- crypto/x509v3/v3_utl.c | 16 ++++++---------- 4 files changed, 21 insertions(+), 25 deletions(-) diff --git a/crypto/x509v3/internal.h b/crypto/x509v3/internal.h index 00dae925b..d07763225 100644 --- a/crypto/x509v3/internal.h +++ b/crypto/x509v3/internal.h @@ -92,9 +92,9 @@ OPENSSL_EXPORT char *x509v3_bytes_to_hex(const uint8_t *in, size_t len); // name, |string_to_hex| converted from hex. unsigned char *x509v3_hex_to_bytes(const char *str, long *len); -// x509v3_name_cmp returns zero if |name| is equal to |cmp| or begins with |cmp| -// followed by '.'. Otherwise, it returns a non-zero number. -int x509v3_name_cmp(const char *name, const char *cmp); +// x509v3_conf_name_matches returns one if |name| is equal to |cmp| or begins +// with |cmp| followed by '.', and zero otherwise. +int x509v3_conf_name_matches(const char *name, const char *cmp); // x509v3_looks_like_dns_name returns one if |in| looks like a DNS name and zero // otherwise. diff --git a/crypto/x509v3/v3_alt.c b/crypto/x509v3/v3_alt.c index 1f83d880b..6123ac3e4 100644 --- a/crypto/x509v3/v3_alt.c +++ b/crypto/x509v3/v3_alt.c @@ -278,7 +278,7 @@ static void *v2i_issuer_alt(const X509V3_EXT_METHOD *method, X509V3_CTX *ctx, } for (i = 0; i < sk_CONF_VALUE_num(nval); i++) { cnf = sk_CONF_VALUE_value(nval, i); - if (!x509v3_name_cmp(cnf->name, "issuer") && cnf->value && + if (x509v3_conf_name_matches(cnf->name, "issuer") && cnf->value && !strcmp(cnf->value, "copy")) { if (!copy_issuer(ctx, gens)) { goto err; @@ -349,12 +349,12 @@ static void *v2i_subject_alt(const X509V3_EXT_METHOD *method, X509V3_CTX *ctx, } for (i = 0; i < sk_CONF_VALUE_num(nval); i++) { cnf = sk_CONF_VALUE_value(nval, i); - if (!x509v3_name_cmp(cnf->name, "email") && cnf->value && + if (x509v3_conf_name_matches(cnf->name, "email") && cnf->value && !strcmp(cnf->value, "copy")) { if (!copy_email(ctx, gens, 0)) { goto err; } - } else if (!x509v3_name_cmp(cnf->name, "email") && cnf->value && + } else if (x509v3_conf_name_matches(cnf->name, "email") && cnf->value && !strcmp(cnf->value, "move")) { if (!copy_email(ctx, gens, 1)) { goto err; @@ -558,19 +558,19 @@ GENERAL_NAME *v2i_GENERAL_NAME_ex(GENERAL_NAME *out, return NULL; } - if (!x509v3_name_cmp(name, "email")) { + if (x509v3_conf_name_matches(name, "email")) { type = GEN_EMAIL; - } else if (!x509v3_name_cmp(name, "URI")) { + } else if (x509v3_conf_name_matches(name, "URI")) { type = GEN_URI; - } else if (!x509v3_name_cmp(name, "DNS")) { + } else if (x509v3_conf_name_matches(name, "DNS")) { type = GEN_DNS; - } else if (!x509v3_name_cmp(name, "RID")) { + } else if (x509v3_conf_name_matches(name, "RID")) { type = GEN_RID; - } else if (!x509v3_name_cmp(name, "IP")) { + } else if (x509v3_conf_name_matches(name, "IP")) { type = GEN_IPADD; - } else if (!x509v3_name_cmp(name, "dirName")) { + } else if (x509v3_conf_name_matches(name, "dirName")) { type = GEN_DIRNAME; - } else if (!x509v3_name_cmp(name, "otherName")) { + } else if (x509v3_conf_name_matches(name, "otherName")) { type = GEN_OTHERNAME; } else { OPENSSL_PUT_ERROR(X509V3, X509V3_R_UNSUPPORTED_OPTION); diff --git a/crypto/x509v3/v3_cpols.c b/crypto/x509v3/v3_cpols.c index 82c68a1ee..5d781c07b 100644 --- a/crypto/x509v3/v3_cpols.c +++ b/crypto/x509v3/v3_cpols.c @@ -241,7 +241,7 @@ static POLICYINFO *policy_section(X509V3_CTX *ctx, } pol->policyid = pobj; - } else if (!x509v3_name_cmp(cnf->name, "CPS")) { + } else if (x509v3_conf_name_matches(cnf->name, "CPS")) { if (!pol->qualifiers) { pol->qualifiers = sk_POLICYQUALINFO_new_null(); } @@ -263,7 +263,7 @@ static POLICYINFO *policy_section(X509V3_CTX *ctx, if (!ASN1_STRING_set(qual->d.cpsuri, cnf->value, strlen(cnf->value))) { goto merr; } - } else if (!x509v3_name_cmp(cnf->name, "userNotice")) { + } else if (x509v3_conf_name_matches(cnf->name, "userNotice")) { STACK_OF(CONF_VALUE) *unot; if (*cnf->value != '@') { OPENSSL_PUT_ERROR(X509V3, X509V3_R_EXPECTED_A_SECTION_NAME); diff --git a/crypto/x509v3/v3_utl.c b/crypto/x509v3/v3_utl.c index d21de4513..466dbf21d 100644 --- a/crypto/x509v3/v3_utl.c +++ b/crypto/x509v3/v3_utl.c @@ -556,18 +556,14 @@ badhex: return NULL; } -int x509v3_name_cmp(const char *name, const char *cmp) { - int len, ret; - char c; - len = strlen(cmp); - if ((ret = strncmp(name, cmp, len))) { - return ret; - } - c = name[len]; - if (!c || (c == '.')) { +int x509v3_conf_name_matches(const char *name, const char *cmp) { + // |name| must begin with |cmp|. + size_t len = strlen(cmp); + if (strncmp(name, cmp, len) != 0) { return 0; } - return 1; + // |name| must either be equal to |cmp| or begin with |cmp|, followed by '.'. + return name[len] == '\0' || name[len] == '.'; } static int sk_strcmp(const char **a, const char **b) { return strcmp(*a, *b); } From 35a31162ffb3a9f6bf7c543e2d516d5cdb339334 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 23 Nov 2022 16:50:17 -0500 Subject: [PATCH 4/4] Switch X509 ex_* flags to uint32_t. The public API already expects them to be uint32_t. Fix the internals to match. Bug: 516 Change-Id: Ia683cc2fac559ebe0b3c7045e4db551224677c28 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55706 Auto-Submit: David Benjamin Commit-Queue: Bob Beck Reviewed-by: Bob Beck --- crypto/x509/internal.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index edba75c81..4cfc4aef6 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -151,10 +151,10 @@ struct x509_st { // These contain copies of various extension values long ex_pathlen; long ex_pcpathlen; - unsigned long ex_flags; - unsigned long ex_kusage; - unsigned long ex_xkusage; - unsigned long ex_nscert; + uint32_t ex_flags; + uint32_t ex_kusage; + uint32_t ex_xkusage; + uint32_t ex_nscert; ASN1_OCTET_STRING *skid; AUTHORITY_KEYID *akid; X509_POLICY_CACHE *policy_cache;