Adding GET support to ocsp app

[Salz, Rich]

The attached diff adds GET support to ocsp. I'd appreciate any feedback.


--
Principal Security Engineer, Akamai Technologies
IM: ***@jabber.me<mailto:***@jabber.me> Twitter: RichSalz

[Erwann Abalea]

(trying a resend, my email address has changed)

Post by Salz, Rich
The attached diff adds GET support to ocsp. I'd appreciate any feedback.

I don't see where the OCSP request is de-base64-ified, and URL-decoded.
In both cases, d2i_OCSP_REQUEST_bio is called to get the request, but
it's done directly on the HTTP request line for a GET.

[Salz, Rich]

Post by Erwann Abalea
I don't see where the OCSP request is de-base64-ified, and URL-decoded.
In both cases, d2i_OCSP_REQUEST_bio is called to get the request, but it's done directly on the HTTP request line for a GET.

Doh! Right :(
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Erwann Abalea]

Post by Salz, Rich
The attached diff adds GET support to ocsp. I'd appreciate any feedback.

I don't see where the OCSP request is de-base64-ified, and URL-decoded.
In both cases, d2i_OCSP_REQUEST_bio is called to get the request, but
it's done directly on the HTTP request line for a GET.

[Salz, Rich]

Post by Erwann Abalea
I don't see where the OCSP request is de-base64-ified, and URL-decoded.
In both cases, d2i_OCSP_REQUEST_bio is called to get the request, but it's done directly on the HTTP request line for a GET.

I forgot to post the updated patch. Thanks Erwann.

--
Principal Security Engineer, Akamai Technologies
IM: ***@jabber.me Twitter: RichSalz

[Erwann Abalea]

Bonjour Rich,

+static char* urldecode(char* p)
+ {
+ unsigned char* out = (unsigned char *)p;
+ char* save = p;
+
+ for ( ; *p; p++)
+ {
+ if (*p == '+')
+ *out++ = ' ';

You're doing "HTML-entity" decoding here. URL decoding uses only the
"%xx" stuff. See RFC3986.

+ else if (*p != '%')
+ *out++ = *p;


[...]

+ }
+ /* URL decode? Really shouldn't be needed. */
+ if (strchr(p, '+') != NULL && strchr(p, '%') != NULL)
+ p = urldecode(p);

URL decode is necessary (RFC2560 says so, RFC3986 lists '+' and '/'
among the reserved characters that need to be encoded). In practice, GET
OCSP requests *are* URL encoded. And if by chance the request isn't
encoded, your test for the presence of a "+" and current urldecode() job
will render this request invalid if is contains a "+" (it can happen in
a Base64 encoded string).
--
Erwann ABALEA

Post by Salz, Rich

Post by Erwann Abalea
I don't see where the OCSP request is de-base64-ified, and URL-decoded.
In both cases, d2i_OCSP_REQUEST_bio is called to get the request, but it's done directly on the HTTP request line for a GET.

I forgot to post the updated patch. Thanks Erwann.

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Salz, Rich]

Post by Erwann Abalea
You're doing "HTML-entity" decoding here. URL decoding uses only the
"%xx" stuff. See RFC3986.
+ else if (*p != '%')
+ *out++ = *p;

Yes, I was treating it as an HTML form, not just a strict URI encoding.

Post by Erwann Abalea
+ /* URL decode? Really shouldn't be needed. */
+ if (strchr(p, '+') != NULL && strchr(p, '%') != NULL)
+ p = urldecode(p);

The comment was misleading and the second test isn't needed (and the && was wrong). So:
/* URL decode? Might not be needed, so check first. */
if (strchr(p, '%') != NULL)
p = urldecode(p);


Thanks again.

So many bugs in such a small piece of code.
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Viktor Dukhovni]

Post by Salz, Rich

Post by Erwann Abalea
You're doing "HTML-entity" decoding here. URL decoding uses only the
"%xx" stuff. See RFC3986.
+ else if (*p != '%')
+ *out++ = *p;

Yes, I was treating it as an HTML form, not just a strict URI encoding.

Decoding "+" as a space applies only in the query part of the URL,
in the base URI, "+" is a literal. Do doing this right requires
a more sophisticated parser.

My incomplete memory of URL syntax is:

http://[user[:pass]@]host[:port/path1/path2/.../pathN[?param1=value1[&...]][#anchor]

with various required encodings to prevent ambiguity that are
context dependent! Parsing URLs correctly requires a bit of care.
Are we trying to extract form data from the URL? What is the goal
here?

Post by Salz, Rich

Post by Erwann Abalea
+ /* URL decode? Really shouldn't be needed. */
+ if (strchr(p, '+') != NULL && strchr(p, '%') != NULL)
+ p = urldecode(p);

/* URL decode? Might not be needed, so check first. */
if (strchr(p, '%') != NULL)
p = urldecode(p);

The decoder does not correctly NUL terminate "p" when it shrinks
by replacing '%xx' with the corresponding octet.

[Salz, Rich]

The decoder does not correctly NUL terminate "p" when it shrinks by
replacing '%xx' with the corresponding octet.

Arrgh. Thanks.

--
Principal Security Engineer, Akamai Technologies
IM: ***@jabber.me Twitter: RichSalz

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Viktor Dukhovni]

+static int tohex(char c)
+ {
+ switch (c)
+ {
+ case '0': return 0;
+ case '1': return 1;
+ case '2': return 2;
+ case '3': return 3;
+ case '4': return 4;
+ case '5': return 5;
+ case '6': return 6;
+ case '7': return 7;
+ case '8': return 8;
+ case '9': return 9;
+ case 'A': case 'a': return 10;
+ case 'B': case 'b': return 11;
+ case 'C': case 'c': return 12;
+ case 'D': case 'd': return 13;
+ case 'E': case 'e': return 14;
+ case 'F': case 'f': return 15;
+ }
+ return 0;
+ }

This code treats non-hex characters as zero, they should trigger
an error.

+ /* URL decode? Really shouldn't be needed. */
+ if (strchr(p, '+') != NULL && strchr(p, '%') != NULL)
+ p = urldecode(p);

That '&&' should be '||'. Is it OK to modify 'p' (aka inbuf) in
place?

+ else if (!strncmp(inbuf, "POST", 4))

This and "GET" case above it should check for a space following
"POST" or "GET".

[Salz, Rich]

This code treats non-hex characters as zero, they should trigger an error.

"Be liberal in what you accept" :) If there's a problem with it, the base64 decode or the DER parse will fail later. If there's not a problem with it, then there.. is no problem.

Post by Erwann Abalea
+ if (strchr(p, '+') != NULL && strchr(p, '%') != NULL)
+ p = urldecode(p);

That '&&' should be '||'. Is it OK to modify 'p' (aka inbuf) in place?

Ouch, you're right! Yes, it's okay to modify it in-place

Post by Erwann Abalea
+ else if (!strncmp(inbuf, "POST", 4))

This and "GET" case above it should check for a space following "POST" or
"GET".

The GET does do the checking because we have to parse the request line. The POST doesn't bother because it's the message body that counts; the ocsp client code doesn't care about the request URI or version. So I think it's okay as-is. In theory someone could say "POSTER" and "fool" the code, but they'd only be fooling themselves. This isn't a general web server, it's only an OCSP responder, so if you don't send a valid OCSP request, it'll reject it anyway.


______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org