Closed Bug 466080 Opened 16 years ago Closed 14 years ago

Make more things honor the LOAD_ANONYMOUS flag

Categories

(Core :: Networking, defect, P2)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.1

People

(Reporter: sicking, Assigned: mayhemer)

References

(Depends on 1 open bug)

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 4 obsolete files)

Test moved to a proper place
(deleted), patch
sicking
: review+
sicking
: superreview+
Details | Diff | Splinter Review
Only tests [Check in comment 67]
(deleted), patch
Details | Diff | Splinter Review 
When adding support for Cross-Site XMLHttpRequest I added a flag to nsIChannel to load the request such that no user-specific data was sent. In other words no cookies or auth headers would be added automatically to the channel by necko.

However there are two forms of authentication that are still automatically added: User-specific SSL certificates and authentication stuff on FTP channels.

Fixing the former is probably a blocker since otherwise the new Cross-Site XHR isn't as secure as it should be. Fixing FTP won't affect any of our shipping code, but might affect any extensions that use the LOAD_ANONYMOUS flag.


Bjarne: Could you look into fixing this? Does the above description make sense? The LOAD_ANONYMOUS flag was added in this patch:
http://hg.mozilla.org/mozilla-central/rev/ca957fbe0ff6
Flags: blocking1.9.1+
Adding test capability dependency. Bug 456001.
Depends on: 456001
Attached patch patch v1 (obsolete) (deleted) — — Splinter Review 
Approach is to avoid setting SSL_GetClientAuthDataHook, causing the browser to not send any certificates to the server. The challenge is really to pass the anonymous-flag from nsHttpChannel down to the PSM-module, but I think changes to the IDLs have been kept at a minimum, only adding two bitflags.

Test-case to be used together with the fix for bug #456001 is coming up...
Attachment #350306 - Flags: review?(jonas)
Attached patch Patch v1.1 plus testcase (obsolete) (deleted) — — Splinter Review 
NSS also had a session-cache and this patch adds a line of code to disable that in case of an XHR-load.

The testcase provided relies on at least patch v2.2 for bug #456001. In order to install and make this work seamlessly, either a workaround or a patch for bug #464191 should be applied. (Workaround can be to manually kill any hanging ssltunnel-processes - if they are left hanging, the new certificates from 456001 will not be used by ssltunnel and this testcase will fail.)

The important parts of this testcase are the iframe loaded via the src-attribute and the last of the tests in the array (load via XHR from secure server requiring a client-cert). The other tests in the array are to check different things not directly related to this patch.

There are three remaining issues we might want to think about

1) Ftp passwords. I suggest to register a separate bug for this to keep things cleaner.

2) What if the JS uses a hidden iframe and manipulates the src-attribute to load a resource from a secure server, instead of using XHR? Is this covered somewhere else or should we try this out as well?

3) We *might* want to extend the test to load the document itself (in an iframe for example) from a secure server requiring client-cert, and then use XHR to load the resource form the same server (i.e. not a cross-site XHR). However, although not covered explicitly, this is probably handled implicitly by the large number of cross-site XHR tests which does not set the ANONYMOUS-flag in this case.
Attachment #350306 - Attachment is obsolete: true
Attachment #351262 - Flags: review?(jonas)
Attachment #350306 - Flags: review?(jonas)
Bjarne: This looks awesome!

The only thing that I think we should test is to make sure that even if there is keep-alive in action, that an XHR load that immediately follows doesn't work.

Another way to test that loading *with* client certs work is by setting the xhr.withCredentials flag to true. That causes the code to not set the LOAD_ANONYMOUS flag. You'd also have to change:

response.setHeader("Access-Control-Allow-Origin", "*", false);
to
response.setHeader("Access-Control-Allow-Origin", request.getHeader("Origin"), false);

This change is safe to make even for the existing tests.
Comment on attachment 351262 [details] [diff] [review]
Patch v1.1 plus testcase

r=me http-channel parts and the tests (with the tests fixed per previous comment).

Please do ask someone that knows the SSL code to review that though.
Attachment #351262 - Flags: superreview+
Attachment #351262 - Flags: review?(jonas)
Attachment #351262 - Flags: review?
Attachment #351262 - Flags: review+
Btw, now I realize... shouldn't we also disable or go somehow over the ssl session cache? If there were a connection made with the server during instance of the browser there is saved ssl session that (AFAIK) remembers the client's credentials if it was provided. So, when we connect the same server again that authenticated session will be reused and LOAD_ANON won't have an effect.
Ah, it's there:

SSL_OptionSet(fd, SSL_NO_CACHE, 1);

Sorry for the spam :)
(In reply to comment #7)
> Ah, it's there:
> 
> SSL_OptionSet(fd, SSL_NO_CACHE, 1);
> 
> Sorry for the spam :)

Np...  I was about to ask you about this, but your comment landed first. :)

Referring to comment #5, who would you suggest to review the SSL code? Could you do it?
Kai Engert is the best one for this but he is quit busy. I could potentially do that or set the flag to someone who would be potentially more appropriate.
Attachment #351262 - Flags: review? → review?(honzab.moz)
(In reply to comment #4)
> The only thing that I think we should test is to make sure that even if there
> is keep-alive in action, that an XHR load that immediately follows doesn't
> work.

Hmm... the sjs was supposed to set keepalive... I'll update the test to use "Connection: Keep-Alive". I don't suppose there is any point in testing it with "Connection: Close" in addition to this...? I mean, if it works with keepalive there is no reason to believe it should fail with close...

> Another way to test that loading *with* client certs work is by setting the
> xhr.withCredentials flag to true. That causes the code to not set the
> LOAD_ANONYMOUS flag.

Not following you... :) Do you mean as a solution to open question 3 from comment #3 ?
 
> You'd also have to change:
> 
> response.setHeader("Access-Control-Allow-Origin", "*", false);
> to
> response.setHeader("Access-Control-Allow-Origin", request.getHeader("Origin"),
> false);
> 
> This change is safe to make even for the existing tests.

Yup - this makes sense in any case. I'll do this in the upcoming patch.
Some caveats about SSL_NO_CACHE:

- this option tells libSSL:
a) if there's already a cached SSL session with this server, do not use 
that cached session for this socket, and 
b) Do not put the new SSL session created for this socket into the cache
where it can be used again.

Now, consider the effect of this on a visit to a web page that has, say,
10 images all coming from the same server.  

In the "normal" case, where SSL_NO_CACHE is NOT used (is set to PR_FALSE), 
the first connection (the one that fetches the html page) establishes an 
SSL session between client and server.  This "full" type of SSL handshake 
uses all the big long slow calculations that take a large part of a second 
on the server and may involve extra round trips between client and server.  
The second and subsequent connections to that same server (e.g. for fetching images) will re-use the SSL session established by that first connection,
and hence will skip all those long slow expensive computations.  The user
(and the server) only pay the price of one "full" handshake, and that cost
is amortized over numerous connections, so the user experiences a delay of
only one single full handshake.  Also, each subsequent visit to this same
server (for some time, measured in hours) will continue to reuse that SSL
session, meaning that pages after the first load even faster than the 
first page, because the first page experiences one full handshake delay,
and subsequent pages experience NO full handshake delay.

In contrast to that, when SSL_NO_CACHE is set to PR_TRUE, it means that 
every single connection between client and server will go through the 
whole "full" handshake.  If each full handshake takes 0.25 seconds, then
10 of them will take 2.5 seconds.  If the server is being hit by many 
clients all doing this, it may get bogged down, and the delay may become
even longer than 2.5 seconds.  This cost will be borne on every page.

Fortunately, there is another approach besides SSL_NO_CACHE that may 
offer the desired effect without causing undesired delay effects.
I'll write about it in a subsequent comment.
(In reply to comment #4)
> The only thing that I think we should test is to make sure that even if there
> is keep-alive in action, that an XHR load that immediately follows doesn't
> work.

Hmm...  Honza just pointed out that httpd.js only supports "connection: close"

Are there any testcases for testing keepalive in general?
NSS implements SSL for both clients and servers.  The SSL session caches
work differently for clients than for servers, so NSS has two separate 
implementations of SSL session caches, one for clients and one for servers.

NSS's client session cache implementation implements multiple, 
individually-named, virtual caches.  When a client application configures
an SSL socket, among the many things it can configure is the name of the 
virtual client session cache that it wishes to use with that socket. 
These individual named virtual caches don't require individual initialization.
To use a new virtual cache with a new name, all the application needs to do 
is make up the new name and configure the socket to use the cache with that 
name, by calling the function SSL_SetSockPeerID.  

There is one "default" (unnamed) virtual cache, and an unlimited number of named virtual caches.  The names are simple utf8 character strings, whose content is entirely up to the application.  

Within each virtual cache, entries are indexed by the server's DNS name, 
IP address (v4 or v6) and port number of the TCP peer (e.g. server) to 
which the socket was connected when the session was established.  The 
DNS name is the string that the application configures into the SSL socket 
by calling SSL_SetURL before initiating the handshake.  Each cached SSL 
session has an expiration time, established when the session is established.
Cached SSL sessions expire automatically when their time is up, or when the
cache is flushed.

Having virtual named caches has a million and one uses.  (OK, maybe that's 
a slight exaggeration, but seriously, it has had WAY more uses since it 
was invented than were ever imagined when it was invented. :) 

The use for which it was originally intended, and for which PSM uses it 
today, is to separate caches for servers reached through a proxy server.
When a proxy server is used, typically all connections for all remote 
servers are made to one single IP address and port number, the address 
and port of the proxy server.  By putting the name and port number of 
the final targer server into the virtual cache name string (also known 
as the "Socket Peer ID" string) the client assures itself that the 
cache entry for one server on one port of one remote host will not 
be reused to attempt to connect to another server on another port on 
that (or any other) remote host.  

But there are many other interesting uses.  Another is this: If a client 
has multiple identities (acting as multiple individual users), each user 
may wish to connect to the same remote server, but have separate SSL sessions 
with that server.  That is easily accomplished by putting the local user
identity into the virtual cache name.  If the application sometimes 
represents Jack and at other times Jill, the caches for Jack and Jill
are easily separated by using their names as part of the virtual cache 
name strings. 

The Anonymous browser may be thought of as a browser in which every page
visit is done by a different user, or that a different user uses the browser 
every minute.  A different virtual cache name may be used for every page 
visit, or every minute.  The same name can be used for all the connections 
associated with that one page (or that one minute), so that the cached SSL 
session is reused when fetching images from the same server as the web page.  
This avoids the cost of a full SSL handshake for every connection (which 
you would get with SSL_NO_CACHE), and achieves appropriate efficiency there, 
without causing the same SSL session to be reused so many times that it 
becomes a source of tracking information.

To combine all these uses, you could make the cache name string include 
all those parts, e.g. a string of the form "<user>:<hostname>:<port>" 
where <user> is "Jack" or "Jill" or "Anonymous page 12345".

For completeness, here is one more detail.  The NSS function SSL_ClearSessionCache clears the entire SSL client session cache, including
all virtual caches.  We don't have a function to clear an individual named
virtual session cache.  If you want such a function, please request it.
(In reply to comment #10)
> (In reply to comment #4)
> > The only thing that I think we should test is to make sure that even if 
> > there is keep-alive in action, that an XHR load that immediately follows 
> > doesn't work.
> 
> Hmm... the sjs was supposed to set keepalive... I'll update the test to use
> "Connection: Keep-Alive". I don't suppose there is any point in testing it 
> with "Connection: Close" in addition to this...? I mean, if it works with 
> keepalive there is no reason to believe it should fail with close...

Yes, just testing *with* keep-alive seems enough. I talked with waldo and it's non-trivial to add keep-alive support to mochitest.

Can you test this manually on some other server for now? If so, we can simply file a bug on writing a keep-alive-enabled test for anonymous channels and make it depend on the bug for implementing keep-alive in httpd.js (there's supposed to be one filed, but I can't find it right now).

> > Another way to test that loading *with* client certs work is by setting the
> > xhr.withCredentials flag to true. That causes the code to not set the
> > LOAD_ANONYMOUS flag.
> 
> Not following you... :) Do you mean as a solution to open question 3 from
> comment #3 ?

Yes. If you do

xhr = new XMLHttpRequest();
xhr.withCredentials = true;
xhr.open("GET", "...", false);
xhr.send();

that'll make a request where LOAD_ANONYMOUS is not set. The server will then have to respond with:

Access-Control-Allow-Origin: http://foo.com
Access-Control-Allow-Credentials: true

in order to allow the request to succeed. You can send this response for all requests.
Keep-alive:

To have keep alive we could potentially hack ssltunnel to behave like a true http(s) proxy and keep a persistent connection with the client if it demands by Connection: keep-alive. In fact, now there is a bug because proxies are not allowed to communicate Connection header in either way. We should remove Connection: close from the server response before we relay it back to the client. It could be easy because there is always just a single response from the server. There is no need to change it to Connection: keep-alive because it is expected by default as I understand related RFC.


SSL session cache:

Nelson, thanks for your full comment and objection. It didn't come through my mind. AllPeers was also using PeerID override because of underlying relay capability using the target peer's name as identifier. As I think of this, it should be enough to use just a single virtual cache for all anonymous requests to any page unless we also don't want a server to potentially detect anonymous requests to different pages coming from a same originator (a same client).
Depends on: 469228
I misunderstood keep-alive; it shouldn't be nearly as much work to implement as I thought, and it's now in the dep list.  I might try to look into that over the weekend or so.
> SSL session cache:
> 
> Nelson, thanks for your full comment and objection. It didn't come through my
> mind. AllPeers was also using PeerID override because of underlying relay
> capability using the target peer's name as identifier. As I think of this, it
> should be enough to use just a single virtual cache for all anonymous requests
> to any page

Yes, seems like you simply want to add a boolean 'is-it-anon-or-not' flag to the cache key currently used. That is exactly what I did for the http cache.

> unless we also don't want a server to potentially detect anonymous
> requests to different pages coming from a same originator (a same client).

I'm not sure I understand this question.
(In reply to comment #13)
> NSS implements SSL for both clients and servers.  The SSL session caches

 [ snip - a long and very useful explanation ]

> where <user> is "Jack" or "Jill" or "Anonymous page 12345".

Thanks a bundle Nelson! This is a much better approach. Will apply it asap.

> For completeness, here is one more detail.  The NSS function
> SSL_ClearSessionCache clears the entire SSL client session cache, including
> all virtual caches.  We don't have a function to clear an individual named
> virtual session cache.  If you want such a function, please request it.

Don't think we need this. The required functionality is that anonymous XHR does not pick up a cached session from a non-anonymous request. Your excellent comments points out that it would be a bad hit to performance if we expire cached sessions for "normal" connections. And finally, as a nice bonus, the outlined solution ensures that sessions for anonymous XHRs are cached separately and are thus reusable by subsequent anonymous XHRs (to the same server and port). Seems ok to me.

To twist it around : Are there reasonable situations where it is useful to clear ssl session caches for anonymous XHR while keeping other session caches?
Attached patch Patch V1.2 (obsolete) (deleted) — — Splinter Review 
Comments from Jonas about tests should be implemented as well as modifications to how we handle the ssl session cache.

(I'm not sure about the exact meaning of all these review-flags - feel free to change my settings...)
Attachment #351262 - Attachment is obsolete: true
Attachment #352733 - Flags: superreview?(nelson)
Attachment #352733 - Flags: review?(jonas)
Attachment #351262 - Flags: review?(honzab.moz)
> Are there reasonable situations where it is useful to clear ssl session 
> caches for anonymous XHR while keeping other session caches?

When I wrote about clearing caches, I was imagining that your objective 
would require you to avoid using the same SSL sessions from one page to 
the next, which is why I suggested putting a page counter (or date/time
in the resolution of minutes would work too) into the virtual cache name.

IF you did that, it would cause the cache to grow to many times its normal 
size, because you'd have a new cache entry for every page on every server 
visited over the course of a day, rather than a new cache entry for every 
server visited over the course of a day, as is the typical case.  This is
because the typical cache lifetime is 24 hours.  We don't presently have
the abillity to specify individual cache lifetimes for cache entries. 
Lifetime of cache entries is a cache-wide parameter, as presently defined.

For a cache entry that is to be used a very few times and then is no 
longer needed, it will then sit in the cache for a long time waiting to
expire, and during that time, it will not be resused.  So, to avoid the
cache swelling out of reasonable bounds, I suggested that you might want
to flush the individually named cache when its useful lifetime is over.

Alternatively, we might want to invent a way for the code that uses the SSL 
socket to specify a cache lifetime for any new SSL session created for that individual socket.  

But, if a single alternative cache, in which the cached sessions may live 
their normal lifetimes, satisfies your requirements, then cache size is 
not a worry, and these other measures are not required.
Attachment #352733 - Flags: superreview?(nelson) → review+
Comment on attachment 352733 [details] [diff] [review]
Patch V1.2

My review comments apply only to the change to function 
nsSSLIOLayerSetOptions that constructs the "peer ID" string 
differently for anonymous connections.  Looks good to me. r+
(In reply to comment #20)
> > Are there reasonable situations where it is useful to clear ssl session 
> > caches for anonymous XHR while keeping other session caches?
> 
> When I wrote about clearing caches, I was imagining that your objective 
> would require you to avoid using the same SSL sessions from one page to 
> the next, which is why I suggested putting a page counter (or date/time
> in the resolution of minutes would work too) into the virtual cache name.

"The fool wonders, the wise man asks" according to Benjamin Disraeli. :) I don't mean to push this too far, but are there situations (relevant to a browser) where this strategy makes sense?

> But, if a single alternative cache, in which the cached sessions may live 
> their normal lifetimes, satisfies your requirements, then cache size is 
> not a worry, and these other measures are not required.

My interpretation of what we want (and of the current code) matches this description. Jonas? Honza? Anyone? Are we safe here or do we need to change more stuff?
> > But, if a single alternative cache, in which the cached sessions may live 
> > their normal lifetimes, satisfies your requirements, then cache size is 
> > not a worry, and these other measures are not required.
> 
> My interpretation of what we want (and of the current code) matches this
> description. Jonas? Honza? Anyone? Are we safe here or do we need to change
> more stuff?

I think the patch is fine. The only thing that we want to ensure is that whenever LOAD_ANONYMOUS is set we connect to the final server without using client certificates (am I getting the wording correct here?).

So it's totally fine to be reusing the same non-client-cert SSL channel for multiple requests to the same server. Even if those requests originate from different pages or indeed different servers.
Hmm...  something just occurred to me :

Setting the 'withCredentials' property on an XHR prevents the LOAD_ANONYMOUS-flag from becoming true (comment #14). nsCrossSiteListenerProxy::CheckRequestApproved() checks that the server accepts this by inspecting response-headers, thus from the XHR's point of view the request fails if the server disagrees.

However, the patch for this bug depends on the LOAD_ANONYMOUS-flag *prior* to receiving the http-response. To be specific : With LOAD_ANONYMOUS=false the NSS-layer will a) re-use any cached session as well as b) automatically send appropriate client-certs in a SSL-handshake. From the XHR's point of view, the request still fails due to nsCrossSiteListenerProxy::CheckRequestApproved(), but credentials have actually been passed to the server.

Should we address this (subtle) issue or will it not represent a problem in practice? It can probably be handled by passing more detailed info to the NSS-layer, but I need to look closer into that.
Depends if we want to be anonymous to the server or to the script that invokes the XHR. The malicious script could a) steal content or cookies returned by the server b) run a request invoking an action on the server like "send money to Mallory's bank account". If the server rejects the request thanks origin access technique implemented on its side then non of this should happen. If not, we allow XSS vulnerable sites be exploitable to complex requests on behalf of the user.

To fix, it might be somehow like this:
- first request should be OPTIONS to determine if the server accepts the request; do this w/ LOAD_ANON set, i.e. no client cert sent
- if the server accepts and has requested a client cert do SSL_SetSockPeerID to use the default cache and SSL_ReHandshake to handshake again with a client cert (we have to carry out a public API probably in nsISSLSocketControl?)
- proceed with the request itself
(In reply to comment #25)
> If not, we allow XSS vulnerable sites be exploitable to complex requests on behalf of the user.

Err, this more leads to a CSRF attack.
(In reply to comment #25)
> Depends if we want to be anonymous to the server or to the script that invokes
> the XHR.

I agree. The crucial question is, I guess : Do we care about being anonymous to the server? If the answer is "no", the current implementation is fine. :)

> To fix, it might be somehow like this:
> - first request should be OPTIONS to determine if the server accepts the
> request; do this w/ LOAD_ANON set, i.e. no client cert sent

Also for preflight-requests the ssl-handshake must happen before the request can be sent to the server. Socket-layer does not/should not know about http-methods and cannot adjust client policies to them. This assumes, of course, that preflight-requests are also done over https - if the user agent is allowed to fall back to clear-text http for the preflight-req we're ok, but I cannot see this stated anywhere. Is this point addressed in the Access-Control spec?

I guess we need Jonas' input on this...
(In reply to comment #27)
> I agree. The crucial question is, I guess : Do we care about being anonymous to
> the server? If the answer is "no", the current implementation is fine. :)
> 

I believe we do care.

> > To fix, it might be somehow like this:
> > - first request should be OPTIONS to determine if the server accepts the
> > request; do this w/ LOAD_ANON set, i.e. no client cert sent
> 
> Also for preflight-requests the ssl-handshake must happen before the request
> can be sent to the server. Socket-layer does not/should not know about
> http-methods and cannot adjust client policies to them. This assumes, of
> course, that preflight-requests are also done over https - if the user agent is
> allowed to fall back to clear-text http for the preflight-req we're ok, but I
> cannot see this stated anywhere. Is this point addressed in the Access-Control
> spec?
> 
> I guess we need Jonas' input on this...

We need more peoples' input on this, it is not that simple. It isn't possible to do http pre-flight and https request; those are two different servers. To clarify my toughs:

- connect https://example.com w/o sending the client cert, using "anon:host:port" peer id, i.e LOAD_ANON set
- send OPTIONS request to check the server accepts the request
- if not, fail the whole XHR
- otherwise, update the underlying ssl socket to use "host:port" cache as in case of non-anonymous load; force renegotiation (SSL_RedoHandshake)
- send the original GET/WHATEVER request

Question is if server would allow this re-handshake and accept the client cert after it, other question is if it would not be simpler to just open a new channel inside of the same XHR instance w/o LOAD_ANON flag set.
> It isn't possible
> to do http pre-flight and https request; those are two different servers.

I think the opposite, namely that it is perfectly feasible to allow http pre-flight requests and https access-requests for a resource. The difference between http and https are really just how the connection is established and the ports used for the connection, but the host and the resource (from the client point-of-view) are exactly the same. However, I'm not convinced that allowing this will fix the problem... :-}

> - connect https://example.com w/o sending the client cert, using
> "anon:host:port" peer id, i.e LOAD_ANON set
> - send OPTIONS request to check the server accepts the request

At the time of establishing the secure connection the server doesn't know that the client is going to send the OPTIONS request. I admit that I don't know the finer details of SSL/TLS, but is it really feasible to allow connections without certs for certain http-methods while requiring certs for other methods? Wouldn't the handshake be done by software unaware of http?

Credentials exist in different layers. Http-auth and cookies exist in the http-layer whereas SSL-certificates exist in the layer below used to transfer http. IMO, in principle it's difficult to use a mechanism in an upper layer to control behaviour in a lower layer.

> We need more peoples' input on this, it is not that simple.

I perfectly agree. :) Let's wait for input before we spend too much time trying to find a solution.
Anyone willing to share their thoughts, in particular on comment #24?
In reaction to comments 24 to 29: I would say we are OK with what we have now. The purpose is to protect against CSRF attacks and not actually be completely anonymous to the server (I have previously said the opposite in comment 28). We can use (send) client certificate authentication when withCredentials is true. In that case the OPTIONS request will be made first anyway. When the server responses with correct Allow-origin header the server is configured to accept the request and there is no security risk from our side even we have already sent the client cert along with the OPTIONS request. When the server doesn't understand the OPTIONS request or ignores it we won't get the Allow-origin header and won't send the main request. So, there is no need to complicate things in this case IMO.
Thanks for clarifying, Honza. :) I also think the answer to "The Crucial Question" in comment #27 is "No", and thus the current approach and proposed patch is fine.

(If anyone, however, answers "Yes" to this question, comments 24-29 outlines a challenge...)

Awaiting Jonas' review.
(In reply to comment #22)
> > But, if a single alternative cache, in which the cached sessions may live 
> > their normal lifetimes, satisfies your requirements, then cache size is 
> > not a worry, and these other measures are not required.
> 
> My interpretation of what we want (and of the current code) matches this
> description. Jonas? Honza? Anyone? Are we safe here or do we need to change
> more stuff?

Yup, that is what we want. The same page could be loading the same url a bit later, or another page could load the same resource. In that case it's great if we can reuse the existing connection.

(In reply to comment #24)
> Hmm...  something just occurred to me :
> 
> Setting the 'withCredentials' property on an XHR prevents the
> LOAD_ANONYMOUS-flag from becoming true (comment #14).
> nsCrossSiteListenerProxy::CheckRequestApproved() checks that the server 
> accepts this by inspecting response-headers, thus from the XHR's point of
> view the request fails if the server disagrees.
> 
> However, the patch for this bug depends on the LOAD_ANONYMOUS-flag *prior* to
> receiving the http-response. To be specific : With LOAD_ANONYMOUS=false the
> NSS-layer will a) re-use any cached session as well as b) automatically send
> appropriate client-certs in a SSL-handshake. From the XHR's point of view, the
> request still fails due to nsCrossSiteListenerProxy::CheckRequestApproved(),
> but credentials have actually been passed to the server.

This is fine and by design.

The way that it works is that we make the GET request, with credentials, to the server hoping that it's going to reply with what we want. If it doesn't reply with what we want we'll simply drop the reply on the floor and tell the requesting page that something failed. No harm done.

There are already tons of ways of causing GET requests to third party sites to happen, the simplest way is to just point a <img src="..."> to any URI and we'll make a request and include appropriate credentials (including using client side certs if it's a https uri). You can't get to the raw returned data, but the request always happens.

For the case when we're worried about the actual request potentially causing harm, the code will make an OPTIONS request first, and only if we get a response there that says that we can make the actual request we'll make the request.

The OPTIONS request happens with the LOAD_ANONYMOUS flag set, the actual request may or may not have the flag set. I take it that if the OPTIONS request has LOAD_ANONYMOUS set, but the actual request does not, we'll end up using different SSL connections for the two requests? If so, that is what we want (at least when there are client certs installed for the second request).

(In reply to comment #27)
> > Depends if we want to be anonymous to the server or to the script that
> > invokes the XHR.
> 
> I agree. The crucial question is, I guess : Do we care about being anonymous
> to the server? If the answer is "no", the current implementation is fine. :)

I'm not sure I follow the question here?

> > To fix, it might be somehow like this:
> > - first request should be OPTIONS to determine if the server accepts the
> > request; do this w/ LOAD_ANON set, i.e. no client cert sent
> 
> Also for preflight-requests the ssl-handshake must happen before the request
> can be sent to the server. Socket-layer does not/should not know about
> http-methods and cannot adjust client policies to them. This assumes, of
> course, that preflight-requests are also done over https - if the user agent 
> is allowed to fall back to clear-text http for the preflight-req we're ok,
> but I cannot see this stated anywhere. Is this point addressed in the
> Access-Control spec?

No, we are not allowed to fallback to http for the preflight. It is considered possible that the http server is managed by one person, but the https server is managed by someone else, so we cant make security decisions for the https server based on what the http server responds.

The socket-layer code won't need to know about which http-method we are using since we are setting the LOAD_ANONYMOUS flag appropriately.

But yes, the preflight needs to happen and finish before we can make the actual request, this happens already in the current code.

(In reply to comment #28)
> We need more peoples' input on this, it is not that simple. It isn't possible
> to do http pre-flight and https request; those are two different servers. To
> clarify my toughs:
> 
> - connect https://example.com w/o sending the client cert, using
> "anon:host:port" peer id, i.e LOAD_ANON set
> - send OPTIONS request to check the server accepts the request
> - if not, fail the whole XHR
> - otherwise, update the underlying ssl socket to use "host:port" cache as in
> case of non-anonymous load; force renegotiation (SSL_RedoHandshake)
> - send the original GET/WHATEVER request

This is what the code does. Except that we don't "update the underlying ssl socket". We simply make a second request which doesn't have the LOAD_ANONYMOUS flag set. As I understand the patch that means that we'll create a new ssl connection to the server, right?


Two additional comments:
1. I don't understand the discussion about being anonymous to the server. With this patch, if LOAD_ANONYMOUS is set on a request, how are we not anonymous? I.e. how could the server figure out who we are. Unless data was intentionally added to the body of the request of course.

2. The way the spec works now the preflight OPTIONS request is always done with LOAD_ANONYMOUS set. This means that we (with this patch) won't use client certificates. We also require that the response to the preflight OPTIONS request contain certain headers, most likely driven by dynamic code. Do you know if servers that use client certs will generally be able to do anything useful with such a request, or are they generally going to immediately going to abort the request, not giving server side CGIs and such the ability to return the required headers.
(In reply to comment #33)
> 1. I don't understand the discussion about being anonymous to the server. With
> this patch, if LOAD_ANONYMOUS is set on a request, how are we not anonymous?
> I.e. how could the server figure out who we are. Unless data was intentionally
> added to the body of the request of course.

My understanding (further explained below) has been that LOAD_ANONYMOUS is *not* set for preflights. The discussion has been about the (rather clumsily expressed) concern in comment #24 : that we identify ourselves to the server at the ssl-level when sending the preflight-request.

(In reply to comment #33)
> 2. The way the spec works now the preflight OPTIONS request is always done with
> LOAD_ANONYMOUS set. This means that we (with this patch) won't use client
> certificates. We also require that the response to the preflight OPTIONS
> request contain certain headers, most likely driven by dynamic code. Do you
> know if servers that use client certs will generally be able to do anything
> useful with such a request, or are they generally going to immediately going to
> abort the request, not giving server side CGIs and such the ability to return
> the required headers.

The following quote from comment #4

> is by setting the
> xhr.withCredentials flag to true. That causes the code to not set the
> LOAD_ANONYMOUS flag.

has made me think that LOAD_ANONYMOUS is *not* set for preflights. Carefully reading code in nsCrossSiteListener indicates that this belief is wrong, and that LOAD_ANONYMOUS is set also for preflights...

However,
the way I understand things : If the server requires ssl-credentials and we *don't* send it, the ssl-connection should not even be established and hence the preflight-request is never sent to the server. Moreover, I really think I have seen LOAD_ANONYMOUS=false when debugging preflights, and this triggered my concern in comment #24. I probably need to trace through the code a few more times to verify or falsify this. Anyone who knows ssl properly - feel free to correct me here!

Further thinking : If we were to be anonymous at the ssl-level for preflights, servers would have to be able to establish credential-less ssl-connections which are *only* capable of transferring http preflight-requests. I'm pretty sure I have not seen anything like that, but I'd be happy if someone tells me I'm wrong and that this is normal.

Summing up : If we don't send ssl-credentials when doing the preflight, I don't understand how we can establish an ssl-connection with a server requiring credentials. And if we do send ssl-credentials, we're clearly identifying ourselves to the server during preflight.
When a server requests client authentication and the client does not 
authenticate itself with a certificate, it's entirely up to the server to
decide what to do next.  The primary options include:
- abort the connection right then and there, or 
- complete the SSL handshake, but treat the client as unauthenticated 
  (This typically means returning a http 401 error page.)

One of those options generates lots of calls to the help desk.  
The other may generate fewer if the error page is heplful.
Nelson: Do you know what existing server software tends to do? What is needed by the Access-Control spec is for the server to complete the ssl handshake and execute CGIs etc as normal, but of course not indicate that any particular user has connected.

(In reply to comment #34)
> The following quote from comment #4
> 
> > is by setting the
> > xhr.withCredentials flag to true. That causes the code to not set the
> > LOAD_ANONYMOUS flag.
> 
> has made me think that LOAD_ANONYMOUS is *not* set for preflights. Carefully
> reading code in nsCrossSiteListener indicates that this belief is wrong, and
> that LOAD_ANONYMOUS is set also for preflights...

When xhr.withCredentials is set to true we *do* want to identify ourselves to the server. Currently (with your patch) we don't during the preflight since the LOAD_ANONYMOUS flag is set, but we do during normal request that follows.

However we might change this, such that we don't set the LOAD_ANONYMOUS flag during preflights when xhr.withCredentials is true. But I don't see that that is a problem. Again, when withCredentials is true, we do intend to identify ourselves to the server in the actual request.
Comment on attachment 352733 [details] [diff] [review]
Patch V1.2

In any case, we want this patch. We might change the nsCrossSiteListenerProxy code to not set the LOAD_ANONYMOUS flag when making the preflight when withCredentials is true, but that'll be a separate bug.
Attachment #352733 - Flags: superreview+
Attachment #352733 - Flags: review?(jonas)
Attachment #352733 - Flags: review+
In comment 35 I wrote:
> - complete the SSL handshake, but treat the client as unauthenticated 
>   (This typically means returning a http 401 error page.)
Either that, or fall back to a name+password form of authentication, 
such as http basic auth, or a form with name and password fields.

In reply to comment 36,
> Nelson: Do you know what existing server software tends to do?
Based on the bugs reports we receive, I'd say it's about evenly divided
between (a) abort (b) fall back to password authentication.  But I would
not say that bug reports are really representative of the distribution of 
behaviors found on the web.  I really don't have a better estimate though.
Thanks for clarifying this, Nelson. :)  I really did not realize that the server actually has a choice if the client refuses to send credentials.

It sounds to me that a server wishing to apply the Access-Control mechanism need to allow at least OPTIONS-requests over credential-less ssl. Not our problem, in other words, since this apparently is possible to do. But as Jonas says, we may choose to relax our code to identify ourselves during preflight if this becomes an issue.

(In reply to comment #36)
> Again, when withCredentials is true, we do intend to identify
> ourselves to the server in the actual request.

Yes, but with malicious JS I guess we cannot assume this?

Anyway, I would like to dbl-check the flags in the debugger before check-in. I did observe some unexpected flags while debugging this and I just want to understand what happens. Give me a day or two to get back on this...
(In reply to comment #39)
> > Again, when withCredentials is true, we do intend to identify
> > ourselves to the server in the actual request.
> 
> Yes, but with malicious JS I guess we cannot assume this?

In the cases where we are worried that the request itself can cause harm on the target server we always do a preflight (that is the whole purpose of the preflight). This is per the Access-Control spec.

So a malicious JS code can't do anything with xhr that it can't do with <img src=>.

Or am I misunderstanding you somehow? If so, could you please describe in detail what attack you are worried about?
(In reply to comment #40)
> In the cases where we are worried that the request itself can cause harm on the
> target server we always do a preflight (that is the whole purpose of the
> preflight). This is per the Access-Control spec.
>
> So a malicious JS code can't do anything with xhr that it can't do with <img
> src=>.
> 
> Or am I misunderstanding you somehow? If so, could you please describe in
> detail what attack you are worried about?

If we sometime later decide to do preflights with LOAD_ANONYMOUS *not* set, we can not support that decision with the argument that a withCredentials-flag means we intend to identify ourselves to the server. But then, it's definitely possible that I am the one not getting things right. :}

I basically think the outlined approach in comments 35-39 is sound. It may cause problems for some servers, but nothing which can't be handled.

Nevertheless, I would like to add a test or two with preflights and check these flags in a debugger again, as mentioned above. I really think I have seen preflights with LOAD_ANONYMOUS not set, causing me to write comment #24 and start this discussion, but I don't recall how it happened. I may be entirely wrong though, but I'd like to check it...
(In reply to comment #41)
> If we sometime later decide to do preflights with LOAD_ANONYMOUS *not* set, we
> can not support that decision with the argument that a withCredentials-flag
> means we intend to identify ourselves to the server.

Why not?
You're right - I'm being paranoid for no reason. This doesn't really matter since the subsequent actual request will reveal our identity anyway...  sorry for causing confusion.
Attached patch Updated with testcase for preflight (obsolete) (deleted) — — Splinter Review 
Added a test to make sure preflights to a server requiring client-cert fails. If we change the current strategy for preflights with respect to LOAD_ANONYMOUS being set, the added test should fail.

Flags have been checked as indicated in comments above and everything seems to be ok - I must have misinterpreted them previously. The patch is ready to go.
Attachment #352733 - Attachment is obsolete: true
Attachment #356708 - Flags: review?(jonas)
Depends on: 473371
Created bug #473371 to address ftp
Attachment #356708 - Flags: superreview+
Attachment #356708 - Flags: review?(jonas)
Attachment #356708 - Flags: review+
Landed this but had to back out due to mochitest failures. See

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1231918070.1231922123.1216.gz

Is the default mochitest test suite set up to test with client certs? Or is something else going wrong?
Ooh, i missed the dependency on bug 456001. Do we need to get that fixed before we can land the tests here?
This bug depends on many other bugs touching the testing suit. It has already been landed but backed out because of test failures. We will soon reland those bugs.
Depends on: 468087, 464191
Target Milestone: --- → mozilla1.9.1
Attached patch Test moved to a proper place (deleted) — — Splinter Review 
Honza discovered that the test from the previous patches is in fact a chrome-test and that it fails when invoked properly as such. The test is not supposed to be a chrome-test, so I moved it and updated it accordingly. Apart from this, the patch is identical to the previous one.
Attachment #356708 - Attachment is obsolete: true
Attachment #357887 - Flags: review?(jonas)
Attachment #357887 - Flags: superreview+
Attachment #357887 - Flags: review?(jonas)
Attachment #357887 - Flags: review+
Are we still waiting for more action in the blocking bugs, or can this land now? Would be great to have in beta3 which means landing within days.
We are still stuck on the ssltunnel kill thing.
Blocks: 474038
Are we?  That bug's fixed, at least on trunk, and soon on branch as well.
Didn't know, sorry.
So, where are we here?  Can we land and close this out?
(In reply to comment #54)
> So, where are we here?  Can we land and close this out?

I'm waiting for review on bug #473371. Bug #456001 is fixed afaik, but it may cause issues for other bugs without my knowledge. Bug #469228 is out of my control.

(This assuming the patch can still be applied to branch - I'm not up to speed on those things. Someone plz let me know if I need to update the patch...)
I landed just the code, no tests, on trunk and 1.9.1 branch
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Hmm.. can we for now land the tests that are in the patch? And write tests for keep-alive later? Don't think we need to fix bug 473371 in order to close this one. We'll use this bug just for the SSL part and separate bugs for other things.
Flags: in-testsuite?
Maybe we should land the test in the patch as well?
Why have not the tests for this bug been landed so far?  I think it's quit important to have them!
> Why have not the tests for this bug been landed so far?  

Because the bug was marked resolved prematurely.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Honza, in the context of your bug about adding tests for client-certs: Have you verified that the "requireclientcert"-hosts in the test-setup actually works? There are two entries in the test for this issue which fails mysteriously and I cannot load the resource manually from "requireclientcert" either - it complains about not being able to verify my cert.
Ahh - didn't see bug #624075 until now. That probably explains it.
Fixed and tested test ready to land.
Assignee: bjarne → honzab.moz
Status: REOPENED → ASSIGNED
Honza, does this test really work for you with a clean tree? It's identical to my unbitrotted version, but none of them can load from the "requireclientcert" host on my box.

Ref also comment #61 and comment #62. I'm adding dependency on bug #624075 - remove it (and plz explain why) if you think this is wrong.
Depends on: 624075
U(In reply to comment #64)
> Honza, does this test really work for you with a clean tree? 
Update your mozilla-central to the latest version, the regression was backed out.  I was testing the test.
No longer depends on: 624075
Ahh - not entirely up-to-date - thanks. :)  Works fine now. Feel free to land patch.
Comment on attachment 502817 [details] [diff] [review]
Only tests [Check in comment 67]

http://hg.mozilla.org/mozilla-central/rev/93b9a60f88fa
Attachment #502817 - Attachment description: Only tests → Only tests [Check in comment 67]
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 14 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
For anyone else that winds up here from comments in the source, bug #1511151 addresses a side-effect of this preventing the client certificate dialog box from being presented in some situations.
Depends on: 1511151
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: