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

RESOLVED FIXED in 3.12.10

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

unspecified
3.12.10

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX 4_3.12.10)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 475733 [details] [diff] [review]
[Not a fix] Add a comment to describe this bug
Attachment #475733 - Flags: review?(alexei.volkov.bugs)
(Assignee)

Updated

8 years ago
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Target Milestone: --- → 3.13
(Assignee)

Comment 2

8 years ago
Created attachment 476447 [details] [diff] [review]
Proposed patch

While investigating a related problem, I wrote a patch for this bug.
Attachment #475733 - Attachment is obsolete: true
Attachment #475733 - Flags: review?(alexei.volkov.bugs)
(Assignee)

Comment 3

8 years ago
Created attachment 476449 [details] [diff] [review]
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.
Attachment #476447 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #476449 - Flags: review?(emaldona)
(Assignee)

Comment 4

8 years ago
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

7 years ago
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
Attachment #476449 - Flags: superreview+

Updated

7 years ago
Whiteboard: PKIX → PKIX 4_3.12.10

Updated

7 years ago
Target Milestone: 3.13 → 3.10

Updated

7 years ago
Priority: P2 → P1

Updated

7 years ago
Attachment #476449 - Flags: review?(emaldona) → review+
Target Milestone: 3.10 → 3.12.10
(Assignee)

Comment 6

7 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

7 years ago
Created attachment 523129 [details] [diff] [review]
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.
Attachment #523129 - Flags: superreview?(alvolkov.bgs)
Attachment #523129 - Flags: review?(emaldona)
(Assignee)

Comment 8

7 years ago
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

7 years ago
Comment on attachment 523129 [details] [diff] [review]
Supplemental patch

unsetting the flags as it's been r+'d by superreviewer and fixed.
Attachment #523129 - Flags: review?(emaldona)
You need to log in before you can comment on or make changes to this bug.