1538980 - SECU_ReadDERFromFile calls strstr on a string that isn't guaranteed to be null-terminated

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

SECStatus
SECU_ReadDERFromFile(SECItem *der, PRFileDesc *inFile, PRBool ascii,
                     PRBool warnOnPrivateKeyInAsciiFile)
{
    SECStatus rv;
    if (ascii) {
        /* First convert ascii to binary */
        SECItem filedata;
        char *asc, *body;

        /* Read in ascii data */
        rv = SECU_FileToItem(&filedata, inFile);
        if (rv != SECSuccess)
            return rv;
        asc = (char *)filedata.data;
        if (!asc) {
            fprintf(stderr, "unable to read data from input file\n");
            return SECFailure;
        }

        if (warnOnPrivateKeyInAsciiFile && strstr(asc, "PRIVATE KEY")) {
            fprintf(stderr, "Warning: ignoring private key. Consider to use "
                            "pk12util.\n");
        }

        /* check for headers and trailers and remove them */
        if ((body = strstr(asc, "-----BEGIN")) != NULL) {
            char *trailer = NULL;
            asc = body;
            body = PORT_Strchr(body, '\n');
            if (!body)
                body = PORT_Strchr(asc, '\r'); /* maybe this is a MAC file */
            if (body)
                trailer = strstr(++body, "-----END");

asc and body point (in) to filedata.data, which gets filled out in SECU_FileToItem and is just the contents of the file. There is no terminating null character. So when SECU_ReadDERFromFile calls strstr and the search term isn't present in the file, this will just read past the end of the space allocated for filedata.data.

I think this is just potential crash/dos, but this can be used to write a zero byte out of bounds, so it might be exploitable (the byte has to be right before the string "-----END", so I imagine this would have to be used in combination with another vulnerability).

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: bug 1538980 - null-terminate ascii input in SECU_ReadDERFromFile so strstr is safe to call r?jcj

Comment 2

[Kevin Jacobs [:kjacobs]]

https://hg.mozilla.org/projects/nss/rev/735ed2e470405ee0159775ef7985959db6e8eb8f