summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSamuel Lidén Borell <samuel@kodafritt.se>2014-02-08 16:28:59 +0100
committerSamuel Lidén Borell <samuel@kodafritt.se>2014-02-08 16:28:59 +0100
commitec90c8fa344379dff8eb8858d41bcfc90caea396 (patch)
treedaed3712c34c8ff59907187471eee00efda4b643
parentdbd66319771f9328b4297b85481962f2835e7f36 (diff)
downloadfribid-ec90c8fa344379dff8eb8858d41bcfc90caea396.tar.gz
fribid-ec90c8fa344379dff8eb8858d41bcfc90caea396.tar.bz2
fribid-ec90c8fa344379dff8eb8858d41bcfc90caea396.zip
More security precautions (length checks, null checks, etc)
The plugin has always limited the input to 10MB per parameter, but this change adds length checks of internal data structures and PKCS7 data, as well as some more null pointer checks.
-rw-r--r--client/certutil.c25
-rw-r--r--client/misc.c24
-rw-r--r--client/misc.h4
-rw-r--r--client/pkcs11.c7
-rw-r--r--client/pkcs12.c15
-rw-r--r--client/request.c3
6 files changed, 62 insertions, 16 deletions
diff --git a/client/certutil.c b/client/certutil.c
index 770b217..43d6757 100644
--- a/client/certutil.c
+++ b/client/certutil.c
@@ -113,6 +113,8 @@ X509_NAME *certutil_parse_dn(const char *s, bool fullDN) {
X509_NAME *subject = X509_NAME_new();
STACK_OF(X509_NAME_ENTRY) *entries = sk_X509_NAME_ENTRY_new_null();
ASN1_OBJECT *obj = NULL;
+ // check for allocation failures and insane name lengths
+ if (!subject || !entries || !checkstrlen(s, 4096)) goto error;
while (*s != '\0') {
// Ignore leading whitespace (this includes whitespace after a comma)
@@ -133,6 +135,7 @@ X509_NAME *certutil_parse_dn(const char *s, bool fullDN) {
// Parse attribute name
char *field = g_strndup(s, nameLength);
+ if (!field) goto error;
int nid;
bool ok = get_non_rfc2256(field, &nid, &obj);
g_free(field);
@@ -157,9 +160,10 @@ X509_NAME *certutil_parse_dn(const char *s, bool fullDN) {
// Add the attributes to the subject name in reverse order
int num = sk_X509_NAME_ENTRY_num(entries);
- for (int i = num; i >= 0; i--) {
+ for (int i = num; i-- > 0; ) {
X509_NAME_ENTRY *entry = sk_X509_NAME_ENTRY_value(entries, i);
- X509_NAME_add_entry(subject, entry, -1, 0);
+ if (!entry) goto error;
+ if (!X509_NAME_add_entry(subject, entry, -1, 0)) goto error;
X509_NAME_ENTRY_free(entry);
}
sk_X509_NAME_ENTRY_free(entries);
@@ -183,6 +187,7 @@ char *certutil_derEncode(X509 *cert) {
char *base64 = NULL;
int len;
+ if (!cert) return NULL;
len = i2d_X509(cert, &der);
if (!der) return NULL;
base64 = base64_encode((const char*)der, len);
@@ -312,7 +317,11 @@ X509 *certutil_findCert(const STACK_OF(X509) *certList,
int num = sk_X509_num(certList);
for (int i = 0; i < num; i++) {
X509 *cert = sk_X509_value(certList, i);
- if (!certutil_compareX509Names(X509_get_subject_name(cert), name, orderMightDiffer) &&
+ if (!cert) continue;
+ X509_NAME *certname = X509_get_subject_name(cert);
+ if (!certname) continue;
+
+ if (!certutil_compareX509Names(certname, name, orderMightDiffer) &&
certutil_hasKeyUsage(cert, keyUsage)) {
return cert;
}
@@ -327,6 +336,8 @@ X509 *certutil_findCert(const STACK_OF(X509) *certList,
*/
bool certutil_addToList(char ***list, size_t *count, X509 *cert) {
+ if (*count >= 100) return false; // insane length for a chain
+
char *certDer = certutil_derEncode(cert);
if (!certDer) goto error;
@@ -357,6 +368,9 @@ void certutil_freeList(char ***list, size_t *count) {
PKCS7 *certutil_parseP7SignedData(const char *p7data, size_t length) {
+ // If the PKCS7 data is bigger than 100 kB then something is really wrong
+ if (!p7data || length > 100*1024) return NULL;
+
// Parse data
const unsigned char *temp = (const unsigned char*)p7data;
PKCS7 *p7 = d2i_PKCS7(NULL, &temp, length);
@@ -393,12 +407,12 @@ char *certutil_getBagAttr(PKCS12_SAFEBAG *bag, ASN1_OBJECT *oid) {
// Find the attribute
ASN1_TYPE *at = NULL;
- if (!bag->attrib) return NULL;
+ if (!bag->attrib || !oid) return NULL;
int numattr = sk_X509_ATTRIBUTE_num(bag->attrib);
for (int i = 0; i < numattr; i++) {
X509_ATTRIBUTE *xattr = sk_X509_ATTRIBUTE_value(bag->attrib, i);
- if (xattr->object && !OBJ_cmp(xattr->object, oid)) {
+ if (xattr && xattr->object && !OBJ_cmp(xattr->object, oid)) {
// Match
at = sk_ASN1_TYPE_value(xattr->value.set, 0);
break;
@@ -409,6 +423,7 @@ char *certutil_getBagAttr(PKCS12_SAFEBAG *bag, ASN1_OBJECT *oid) {
// Copy the value to a string
int len = at->value.printablestring->length;
+ if (len < 0 || len > 100*1024) return NULL;
char *str = malloc(len+1);
if (str) {
memcpy(str, at->value.printablestring->data, len);
diff --git a/client/misc.c b/client/misc.c
index 6c6fb9b..834de1d 100644
--- a/client/misc.c
+++ b/client/misc.c
@@ -1,6 +1,6 @@
/*
- Copyright (c) 2009-2011 Samuel Lidén Borell <samuel@kodafritt.se>
+ Copyright (c) 2009-2014 Samuel Lidén Borell <samuel@kodafritt.se>
Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
@@ -32,6 +32,9 @@
#include "misc.h"
+/* Max size is 20 MB */
+#define MAX_SANE_DATA_SIZE 20*1024*1024
+
/**
* Like sprintf, but allocates and returns a string instead of
* using a pre-allocated buffer.
@@ -63,6 +66,7 @@ char *rasprintf_append(char *str, const char *format, ...) {
size_t oldlen = strlen(str);
size_t taillen = strlen(tail);
+ if (oldlen > MAX_SANE_DATA_SIZE || taillen > MAX_SANE_DATA_SIZE) goto error;
char *merged = realloc(str, oldlen+taillen+1);
if (!merged) goto error;
@@ -152,6 +156,8 @@ char *base64_decode(const char *encoded) {
char *temp = (char*)g_base64_decode(encoded, &length);
if (!temp) goto error;
+ if (length > MAX_SANE_DATA_SIZE) goto error;
+
char *result = malloc(length+1);
if (!result) goto error;
@@ -172,8 +178,7 @@ char *base64_decode_binary(const char *encoded, size_t *decodedLength) {
char *result = (char*)g_base64_decode(encoded, &length);
*decodedLength = length;
- if (*decodedLength != length) {
- // Integer overflow
+ if (length > MAX_SANE_DATA_SIZE) {
free(result);
return NULL;
}
@@ -233,3 +238,16 @@ bool is_valid_hostname(const char *hostname) {
bool is_https_url(const char *url) {
return !strncmp(url, "https://", 8);
}
+
+/**
+ * Returns true if the string is at most maxlen bytes,
+ * including the null terminator.
+ */
+bool checkstrlen(const char *s, size_t maxlen) {
+ while (maxlen) {
+ if (!*s) return true;
+ s++; maxlen--;
+ }
+ return false;
+}
+
diff --git a/client/misc.h b/client/misc.h
index 4dae11e..14e4281 100644
--- a/client/misc.h
+++ b/client/misc.h
@@ -1,6 +1,6 @@
/*
- Copyright (c) 2009-2011 Samuel Lidén Borell <samuel@kodafritt.se>
+ Copyright (c) 2009-2014 Samuel Lidén Borell <samuel@kodafritt.se>
Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
@@ -54,6 +54,8 @@ bool is_valid_ip_address(const char *ip);
bool is_valid_hostname(const char *hostname);
bool is_https_url(const char *url);
+bool checkstrlen(const char *s, size_t maxlen);
+
#endif
diff --git a/client/pkcs11.c b/client/pkcs11.c
index df49969..94c769c 100644
--- a/client/pkcs11.c
+++ b/client/pkcs11.c
@@ -121,9 +121,10 @@ static TokenError _backend_sign(PKCS11Token *token,
const char *message, size_t messagelen,
char **signature, size_t *siglen) {
- assert(message != NULL);
- assert(signature != NULL);
- assert(siglen != NULL);
+ if (!message || !signature || !siglen) {
+ assert(false);
+ return TokenError_Unknown;
+ }
if (messagelen >= UINT_MAX) return TokenError_MessageTooLong;
diff --git a/client/pkcs12.c b/client/pkcs12.c
index 8b55be1..4eb10ae 100644
--- a/client/pkcs12.c
+++ b/client/pkcs12.c
@@ -213,6 +213,7 @@ static STACK_OF(X509) *pkcs12_listCerts(PKCS12 *p12) {
*/
static PKCS12Token *createToken(const Backend *backend, SharedPKCS12 *sharedP12,
X509_NAME *id, void *tag) {
+ if (!sharedP12 || !id) return NULL;
PKCS12Token *token = calloc(1, sizeof(PKCS12Token));
if (!token) return NULL;
token->base.backend = backend;
@@ -240,7 +241,10 @@ static TokenError _backend_addFile(Backend *backend,
if (!p12) return TokenError_BadFile;
STACK_OF(X509) *certList = pkcs12_listCerts(p12->data);
- if (!certList) return TokenError_Unknown;
+ if (!certList) {
+ pkcs12_release(p12);
+ return TokenError_Unknown;
+ }
int certCount = sk_X509_num(certList);
for (int i = 0; i < certCount; i++) {
@@ -310,9 +314,10 @@ static TokenError _backend_sign(PKCS12Token *token,
const char *message, size_t messagelen,
char **signature, size_t *siglen) {
- assert(message != NULL);
- assert(signature != NULL);
- assert(siglen != NULL);
+ if (!message || !signature || !siglen) {
+ assert(false);
+ return TokenError_Unknown;
+ }
if (messagelen >= UINT_MAX) return TokenError_MessageTooLong;
@@ -506,6 +511,7 @@ TokenError _backend_createRequest(const RegutilInfo *info,
bool ok = true;
CertReq *reqs = NULL;
STACK *x509reqs = sk_new_null();
+ if (!x509reqs) return TokenError_Unknown;
for (const RegutilPKCS10 *pkcs10 = info->pkcs10; pkcs10 != NULL;
pkcs10 = pkcs10->next) {
@@ -568,6 +574,7 @@ TokenError _backend_createRequest(const RegutilInfo *info,
// Store in list
CertReq *req = malloc(sizeof(CertReq));
+ if (!req) goto req_error;
req->pkcs10 = pkcs10;
req->privkey = privkey;
req->rsa = rsa;
diff --git a/client/request.c b/client/request.c
index b46be3b..9655bc6 100644
--- a/client/request.c
+++ b/client/request.c
@@ -128,12 +128,14 @@ IMPLEMENT_ASN1_FUNCTIONS(PKIDATA)
static ASN1_INTEGER *intToAsn1(int i) {
ASN1_INTEGER *a = ASN1_INTEGER_new();
+ if (!a) abort();
ASN1_INTEGER_set(a, i);
return a;
}
static ASN1_IA5STRING *strToIA5(const char *s) {
ASN1_IA5STRING *a = ASN1_IA5STRING_new();
+ if (!a) abort();
ASN1_STRING_set((ASN1_STRING*)a, s, strlen(s));
return a;
}
@@ -182,6 +184,7 @@ void request_wrap(STACK *reqs, char **der, size_t *derLength) {
int num = sk_num(reqs);
for (int i = 0; i < num; i++) {
X509_REQ *req = (X509_REQ*)sk_value(reqs, i);
+ if (!req) goto end;
REQ_BODY_PART *reqPart = wrapBodyPartReq(req, 0x01000002+i);