Skip to content

Simplify X.509 extension handling code #1855

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

Merged
alex merged 2 commits into from
Jun 6, 2017
Merged

Simplify X.509 extension handling code #1855

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dce052d
Simplify X.509 extension handling code
alex May 29, 2017
7eebbaa
Delete even more code
alex May 29, 2017
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
34 changes: 4 additions & 30 deletions Modules/_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,57 +905,31 @@ _get_peer_alt_names (X509 *certificate) {
then iterates through the stack to add the
names. */

int i, j;
int j;
PyObject *peer_alt_names = Py_None;
PyObject *v = NULL, *t;
X509_EXTENSION *ext = NULL;
GENERAL_NAMES *names = NULL;
GENERAL_NAME *name;
const X509V3_EXT_METHOD *method;
BIO *biobuf = NULL;
char buf[2048];
char *vptr;
int len;
const unsigned char *p;

if (certificate == NULL)
return peer_alt_names;

/* get a memory buffer */
biobuf = BIO_new(BIO_s_mem());

i = -1;
while ((i = X509_get_ext_by_NID(
certificate, NID_subject_alt_name, i)) >= 0) {

names = (GENERAL_NAMES *)X509_get_ext_d2i(
certificate, NID_subject_alt_name, NULL, NULL);
Copy link
Copy Markdown

@reaperhulk reaperhulk Jun 1, 2017

Choose a reason for hiding this comment

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

Do we care about handling the case where there is > 1 SAN extension? Right now this will return the first one found and use it, but if there is > 1 we should really just error out.

Copy link
Copy Markdown
Member Author

@alex alex Jun 1, 2017

Choose a reason for hiding this comment

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

None of the other X.509 extension handlers attempt to handle this; we just defer to OpenSSL's behavior.

Copy link
Copy Markdown

@reaperhulk reaperhulk Jun 1, 2017

Choose a reason for hiding this comment

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

I think this actually Does The Right Thing, the docs are just profoundly awful:

"If idx is NULL then only one occurrence of an extension is permissible otherwise the first extension after index *idx is returned and *idx updated to the location of the extension. If crit is not NULL then *crit is set to a status value: -2 if the extension occurs multiple times (this is only returned if idx is NULL), -1 if the extension could not be found, 0 if the extension is found and is not critical and 1 if critical. A pointer to an extension specific structure or NULL is returned."

if (names != NULL) {
if (peer_alt_names == Py_None) {
peer_alt_names = PyList_New(0);
if (peer_alt_names == NULL)
goto fail;
}

/* now decode the altName */
ext = X509_get_ext(certificate, i);
if(!(method = X509V3_EXT_get(ext))) {
PyErr_SetString
(PySSLErrorObject,
ERRSTR("No method for internalizing subjectAltName!"));
goto fail;
}

p = X509_EXTENSION_get_data(ext)->data;
if (method->it)
names = (GENERAL_NAMES*)
(ASN1_item_d2i(NULL,
&p,
X509_EXTENSION_get_data(ext)->length,
ASN1_ITEM_ptr(method->it)));
else
names = (GENERAL_NAMES*)
(method->d2i(NULL,
&p,
X509_EXTENSION_get_data(ext)->length));

for(j = 0; j < sk_GENERAL_NAME_num(names); j++) {
/* get a rendering of each name in the set of names */
int gntype;
Expand Down