Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable loading of unprotected credentials from PKCS#12 files. #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Disable loading of unprotected credentials from PKCS#12 files. #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 168 additions & 24 deletions src/storage/files.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,11 @@ typedef struct pw_cb_data

static int password_callback(char* buf, int bufsiz, int verify, void* cb_tmp)
{
if(0 == buf || bufsiz <= 0 || 0 == cb_tmp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you you forbid 0 == cb_tmp ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better move the checks on buf and bufsiz below into if(password not_eq 0) {
because they are needed only in this case.

{
return -1;
}

int res = 0 * verify; /* make (artificial) use of 'verify' */
const char* password = 0;
PW_CB_DATA* cb_data = (PW_CB_DATA*)cb_tmp;
Expand All @@ -339,11 +344,145 @@ static int password_callback(char* buf, int bufsiz, int verify, void* cb_tmp)
{
res = bufsiz;
}
memcpy(buf, password, res); /* copy password and length(res) into buf */
memcpy(buf, password, (size_t)res); /* copy password and length(res) into buf */
}
return res; /* the size */
}


static int get_algo_nid(const X509_ALGOR *alg)
{
int pbenid, aparamtype;
const ASN1_OBJECT *aoid;
const void *aparam;

X509_ALGOR_get0(&aoid, &aparamtype, &aparam, alg);

pbenid = OBJ_obj2nid(aoid);

if(pbenid == NID_pbes2)
{
PBE2PARAM *pbe2 = 0;
int encnid;
if(aparamtype == V_ASN1_SEQUENCE)
{
pbe2 = ASN1_item_unpack(aparam, ASN1_ITEM_rptr(PBE2PARAM));
}
if(pbe2 == 0)
{
BIO_puts(bio_err, ", <unsupported parameters>");
return NID_undef;
}
X509_ALGOR_get0(&aoid, &aparamtype, &aparam, pbe2->keyfunc);
pbenid = OBJ_obj2nid(aoid);
X509_ALGOR_get0(&aoid, 0, 0, pbe2->encryption);
encnid = OBJ_obj2nid(aoid);
PBE2PARAM_free(pbe2);
return encnid;
}

return NID_undef;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really justified to return ID_undef for all other cases, i.e. pbenid != NID_pbes2?
If you, add a comment why.

}


static int bag_get_pkey_algo(const PKCS12_SAFEBAG *bag, const char *pass, int passlen)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass and passlen are unused, so remove them from the function parameter list.

{
// we only support a shrouded keybag
if(PKCS12_SAFEBAG_get_nid(bag) is_eq NID_pkcs8ShroudedKeyBag)
{
const X509_SIG *tp8;
const X509_ALGOR *tp8alg;

tp8 = PKCS12_SAFEBAG_get0_pkcs8(bag);
X509_SIG_get0(tp8, &tp8alg, 0);
return get_algo_nid(tp8alg);
}
return NID_undef;
}


static int bags_get_pkey_algo(const STACK_OF(PKCS12_SAFEBAG) *bags, const char *pass, int passlen, int desired_algo_nid)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass and passlen are effectively unused, so remove them from the function parameter list.

{
int i, algo_nid = NID_undef;
for(i = 0; i < sk_PKCS12_SAFEBAG_num(bags); ++i)
{
int bag_algo_nid = bag_get_pkey_algo(sk_PKCS12_SAFEBAG_value(bags, i), pass, passlen);
if(bag_algo_nid is_eq NID_undef)
{
continue;
}
if(bag_algo_nid not_eq desired_algo_nid)
{
return bag_algo_nid;
}
algo_nid = bag_algo_nid;
}
return algo_nid;
}


/* This function determines whether a given PKCS#12 file contains at least one private key that was encrypted using the
desired algorithm. */
/* Iterates over all keybags in the file but only looks inside Shrouded keybags. Returns 1 if the desired algorithm
and no other was found, 0 otherwise. */
static int algo_allowed(const PKCS12* p12, const char* pass, int passlen, int desired_algo_nid)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better: static bool algo_allowed(...) to point out that the result is Boolean
and use false and true rather than 0 and 1 for Boolean values below.

{
STACK_OF(PKCS7) *auth_safes = 0;
STACK_OF(PKCS12_SAFEBAG) *bags = 0;
int i, bagnid;
int found_desired_algo = 0;
PKCS7 *p7 = 0;

auth_safes = PKCS12_unpack_authsafes(p12);

if(0 is_eq auth_safes)
{
return 0;
}

for(i = 0; i < sk_PKCS7_num(auth_safes); ++i)
{
p7 = sk_PKCS7_value(auth_safes, i);
bagnid = OBJ_obj2nid(p7->type);
if(bagnid == NID_pkcs7_data)
{
bags = PKCS12_unpack_p7data(p7);
}
else if(bagnid == NID_pkcs7_encrypted)
{
bags = PKCS12_unpack_p7encdata(p7, pass, passlen);
}
else
{
continue;
}
if(!bags)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better: 0 is_eq bags

{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simpler: goto line 482.

sk_PKCS7_pop_free(auth_safes, PKCS7_free);
return 0;
}

int algo = bags_get_pkey_algo(bags, pass, passlen, desired_algo_nid);
sk_PKCS12_SAFEBAG_pop_free(bags, PKCS12_SAFEBAG_free);
bags = 0;

if(algo is_eq NID_undef)
{
continue;
}
if(algo is_eq desired_algo_nid)
{
found_desired_algo = 1;
continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why continue here?
You can break since all you wanted is find at least one such key algo.

}
sk_PKCS7_pop_free(auth_safes, PKCS7_free);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simpler: goto line 482.

return 0;
}

sk_PKCS7_pop_free(auth_safes, PKCS7_free);
return found_desired_algo;
}

/* adapted from OpenSSL:apps/lib/apps.c
* @warning security note: the `pem_cb` callback must be compliant with
* https://wiki.sei.cmu.edu/confluence/display/c/FIO30-C.+Exclude+user+input+from+format+strings
Expand Down Expand Up @@ -371,32 +510,37 @@ static int load_pkcs12(BIO* in, OPTIONAL const char* desc, OPTIONAL pem_password
/* See if an empty password will do */
if(PKCS12_verify_mac(p12, "", 0) not_eq 0 or PKCS12_verify_mac(p12, 0, 0) not_eq 0)
{
LOG(FL_WARN, "Unencrypted PKCS#12 file%s%s", for_str, desc_str);
pass = "";
LOG(FL_WARN, "Rejecting unencrypted PKCS#12 file%s%s", for_str, desc_str);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FL_WARN -> FL_ERR, if this case is really considered an error, but see below.

return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does not really do what is given it is description.
It rejects PKCS#12 not having a password even if the password provided by the caller is empty or no password was given by the caller. This is too much of a restriction.
In the cases just described, you can give a warning if wanted, but do not stop with an error. Otherwise, this library can no more be used to handle PKCS#12 files that (possibly for a good reason) are not password-protected.

}
else
if(pem_cb is_eq 0)
{
if(pem_cb is_eq 0)
{
pem_cb = password_callback;
}
len = pem_cb(tpass, PEM_BUFSIZE, 0, cb_data);
if(len < 0)
{
LOG(FL_ERR, "passphrase callback error for %s", desc not_eq 0 ? desc : "PKCS#12 file");
goto die;
}
if(len < PEM_BUFSIZE)
{
tpass[len] = 0;
}
if(0 is_eq PKCS12_verify_mac(p12, tpass, len))
{
LOG(FL_ERR, "mac verify error (wrong password?) in PKCS12 file%s%s", for_str, desc_str);
goto die;
}
pass = tpass;
pem_cb = password_callback;
}
len = pem_cb(tpass, PEM_BUFSIZE, 0, cb_data);
if(len < 0)
{
LOG(FL_ERR, "passphrase callback error for %s", desc not_eq 0 ? desc : "PKCS#12 file");
goto die;
}
if(len < PEM_BUFSIZE)
{
tpass[len] = 0;
}
if(0 is_eq PKCS12_verify_mac(p12, tpass, len))
{
LOG(FL_ERR, "mac verify error (wrong password?) in PKCS12 file%s%s", for_str, desc_str);
goto die;
}

pass = tpass;

if(0 is_eq algo_allowed(p12, pass, len, NID_aes_256_cbc))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder about the strange - and not very readable - way of checking a Boolean (return) value here and at many other places. Better use if(not algo_allowed(...))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you hard-coded NID_aes_256_cbc, but our PR description says
"the same algorithm that is used by "storage" functions (e.g. CREDENTIALS_save_dv)".
Instead, you should define a symbolic name for this algorithm and use it in the code where appropriate in order to prevent (future) inconsistencies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algo check must not be made mandatory because otherwise general usability of PKCS#12 loading is hampered.

{
LOG(FL_ERR, "Rejecting PKCS#12 file%s%s due to unsupported private key encryption algorithm", for_str, desc_str);
return 0;
}

EVP_PKEY* unused_pkey = 0;
X509* unused_cert = 0;
ret = PKCS12_parse(p12, pass, pkey is_eq 0 ? &unused_pkey : pkey, cert is_eq 0 ? &unused_cert : cert, ca);
Expand Down