596842 - pkix_pl_InfoAccess_ParseLocation should replace "%" hex hex escape sequences before calling pkix_pl_InfoAccess_ParseTokens.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

The pkix_pl_InfoAccess_ParseLocation function in
lib/libpkix/pkix_pl_nss/pki/pkix_pl_infoaccess.c
parses an LDAP URL.  It has two problems.

1. (Nit) The function's name should contain "LDAP"
because the function is specific to LDAP.  (The
related pkix_pl_InfoAccess_ParseTokens function has
the same problem.)

2. (Subject of this bug) The function assumes that
space (' ') is the only character represented by a
"%" hex hex escape sequence in LDAP URLs.  This
assumption is wrong.  In bug 595264 comment 1 I
showed two LDAP URLs that also escapes '=' (%3d)
and ',' (%2c).  One of them is processed by the
pkix_pl_InfoAccess_ParseLocation function:

ldap://crl.gds.disa.mil/cn%3dDoD%20Root%20CA%202%2cou%3dPKI%2cou%3dDoD%2co%3dU.S.%20Government%2cc%3dUS?cACertificate;binary

pkix_pl_InfoAccess_ParseLocation should replace
"%" hex hex escape sequences before calling
pkix_pl_InfoAccess_ParseTokens with ',' as the
separator here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_infoaccess.c&rev=1.8&mark=800#795

After we fix this, we should be able to remove the
code in pkix_pl_InfoAccess_ParseTokens that replaces
"%20" by a space ' ':

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_infoaccess.c&rev=1.8&mark=632-639#631

Since I don't know how to test with LDAP, I didn't try to
fix this bug.

Comment 1

[Wan-Teh Chang]

Attachment: [Not a fix] Add a comment to describe this bug

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch

While investigating a related problem, I wrote a patch for this bug.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch v1.1

Since my patch modifies the 'locationAscii' string in place,
I deleted the commented-out test code, which points 'locationAscii'
to a const string.

Comment 4

[Wan-Teh Chang]

My patch for this bug adds a function that unescapes the "%" hex hex
escape sequences (such as %20 for a space) in URLs.

Does anyone know if NSS already has such a function?

Comment 5

[Alexei Volkov]

Comment on attachment 476449 [details] [diff] [review]
Proposed patch v1.1

Elio, please review this patch. It needs second review to be integrated into 3.12.10. Thx

Comment 6

[Wan-Teh Chang]

Patch checked in on the NSS trunk (NSS 3.13) and NSS_3_12_BRANCH
(NSS 3.12.10).

Checking in pkix_pl_infoaccess.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_infoaccess.c,v  <--  pkix_pl_infoaccess.c
new revision: 1.10; previous revision: 1.9
done

Checking in pkix_pl_infoaccess.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_infoaccess.c,v  <--  pkix_pl_infoaccess.c
new revision: 1.8.4.2; previous revision: 1.8.4.1
done

Comment 7

[Wan-Teh Chang]

Attachment: Supplemental patch

This patch improves pkix_pl_InfoAccess_ParseTokens.

1. Declare the local variables 'len' and 'p' in the smallest
lexical scope in which they're used.  This makes it easier
to reason about them.

2. Before we dereference *(endPos-1), check that endPos > *startPos.
Otherwise we may read before *startPos.  Also, use the 'separator'
argument instead of the hardcoded ','.  (We always pass ',' as the
'separator' argument, so this is for generality.)

3. Do not increment 'p' by 'len'.  Simply use p[len].

4. When we break out of the while loop because endPos == '\0',
set *startPos = endPos.  This matches the original code, even
though this may not be necessary.  This ensures that the next
pkix_pl_InfoAccess_ParseTokens call won't scan what has been
scanned.

Comment 8

[Wan-Teh Chang]

Comment on attachment 523129 [details] [diff] [review]
Supplemental patch

Checked in on the NSS trunk (NSS 3.13) and NSS_3_12_BRANCH (NSS 3.12.10).

Checking in pkix_pl_infoaccess.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_infoaccess.c,v
  <--  pkix_pl_infoaccess.c
new revision: 1.11; previous revision: 1.10
done

Checking in pkix_pl_infoaccess.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_infoaccess.c,v
  <--  pkix_pl_infoaccess.c
new revision: 1.8.4.3; previous revision: 1.8.4.2
done

Comment 9

[Elio Maldonado]

Comment on attachment 523129 [details] [diff] [review]
Supplemental patch

unsetting the flags as it's been r+'d by superreviewer and fixed.