Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 159343 HSTS does not work properly with POST requests
Starred by 3 users Reported by kypri...@gmail.com, Nov 4 2012 Back to list
Status: Fixed
Owner:
Closed: Nov 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment
UserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0

Steps to reproduce the problem:
1. Configure some host to use HSTS
for nginx I used:
add_header Strict-Transport-Security "max-age=86400";

2. Access it via HTTPS to apply HSTS policy
3. Use the form attached and submit POST request to HTTP (hsts_post.html)

What is the expected behavior?
Request rewritten to HTTPS and POST data retransmitted (hsts_correct.png)

What went wrong?
Request rewritten to HTTPS, POST data discarded, method changed to GET (hsts_bug.png)

Example URL:
https://ctftime.org/test_post.php

Did this work before? No 

Chrome version: 22.0.1229.96 m  Channel: stable
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)

If you need any other information, please let me know.
 
hsts_post.html
175 bytes View Download
hsts_bug.PNG
10.5 KB View Download
hsts_correct.PNG
2.5 KB View Download
Labels: Internals-Network-HTTP
Owner: palmer@chromium.org
Status: Assigned
palmer: assigning to you due to recent work on HSTS correctness.
Cc: michaeln@chromium.org shalev@chromium.org rsleevi@chromium.org
Labels: -Pri-2 -OS-Windows Pri-1 OS-All
I don't think the problem is from any of my changes, but I could be wrong!

I think the problem is that 

1. When we GetHSTSRedirect, we create a URLRequestRedirectJob instead of a URLRequestHttpJob. From net/url_request/url_request_http_job.cc:

 211   GURL redirect_url;
 212   if (request->GetHSTSRedirect(&redirect_url))
 213     return new URLRequestRedirectJob(request, network_delegate, redirect_url);
 214   return new URLRequestHttpJob(request, network_delegate);

2. URLRequestRedirectJob::IsRedirectResponse always returns true

3. URLRequestRedirectJob::IsRedirectResponse's callers interpret that to mean they should do a GET, instead of whatever method the original request was

I suspect URLRequestRedirectJob is not the right tool for the job of redirecting HSTS — it seems to want to be used for actual 3xx responses. Perhaps we should do the work that GetHSTSRedirect does directly on request.url, and then fire off a URLRequestHttpJob as normal. But, that doesn't seem possible; once you create your URLRequest, its url_chain_ is set in stone (const).

The other option would seem to be informing URLRequestRedirectJob about the nature of this kind of redirect, and the need to help its callers not throw away the original request's method or post data.

I think. Any clues appreciated...
Cc: willchan@chromium.org
palmer: Everything you just described sounds like a correct description of the problem.

I suspect that a URLRequest::Intercepter may be correct. I'll chat with willchan this afternoon to confirm.
Cc: mmenke@chromium.org
There are redirects like 307 which preserve the POST method. Deferring to willchan or mmenke about the appropriate architecture here.
URLRequestRedirectJobs simulate 302 redirects - the constructor sets http_status_code_ to 302.  A fix may be as simple as switching it to a 307.  I'm not sure if other consumers expect 302s, though, so would probably be safer to add an argument to the constructor.
I tried switching it to 307 (by using a new constructor that takes a StatusCode argument), but that doesn't solve it. URLRequestJob::NotifyHeadersComplete still goes into the defer_redirect case, which I think? might be the problem. But I could be wrong about that.

I don't know this code super well; I wonder if a real net/ team member can solve this faster than I can?
Owner: mmenke@chromium.org
I'm happy to take it.  Not sure if I can fix it faster, but I'll probably learn more than I should know anyways in the process of doing so.  :)
Cc: palmer@chromium.org
Oops...Forgot to add you back to the cc list:  palmer, I'll take the bug off your hands.
Cool, thanks. :)
I do not want to use an Interceptor here. Let's fix it in the HttpProtocolHandler or whatever. The reason is because Interceptors are designed for optional hooking of the network stack. I believe that this belongs in the HTTP stack proper, so we should bake the code into the HttpProtocolHandler. I'm fine with fixing URLRequestRedirectJob to make this work or whatever. I do think that using a URLRequestJob of some sort is the right strategy, and would like it to be logged appropriately in net-internals.
palmer:  How did you test this?  I just changed the redirect code used by URLRequestRedirectJob, and it seems to fix the issue for me.  Want to make sure I'm not missing some case.
I made a simple form that tries to POST to http://my-workstation/foo.html, set an HSTS rule for my-workstation, and watched Chrome defer the POST and do a GET to https://my-workstation/foo.html in the Network tab of Developer Tools.

Perhaps you are better at changing the redirect code used by URLRequestRedirectJob than I am. :)
Or worse at setting up an HTTPS server.

I just did an empty POST to www.google.com, and made sure I got a complaint about a POST on an SSL page instead of an HTTP one, and set break points to double-check we were sending a POST.

I'll add unit tests and plan to land as-is, after a little more double-checking out of paranoia.
palmer:  Actually, looks like you know what you're doing.  Devtools just doesn't like URLRequestRedirectJobs.  I can verify that we're sending a POST in net-internals, but devtools claims it's a GET.  It's correctly reading all headers, and gets the response status code right, just doesn't get request method correct.
Is there a separate bug for the devtools reporting issue?
Comment 16 by kypri...@gmail.com, Nov 27 2012
Any ETA?
Project Member Comment 17 by bugdroid1@chromium.org, Nov 28 2012
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=170029

------------------------------------------------------------------------
r170029 | mmenke@chromium.org | 2012-11-28T19:52:14.376296Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/url_request/url_request_http_job.cc?r1=170029&r2=170028&pathrev=170029
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/url_request/url_request_redirect_job.cc?r1=170029&r2=170028&pathrev=170029
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chromeos/gview_request_interceptor.cc?r1=170029&r2=170028&pathrev=170029
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/url_request/url_request_redirect_job.h?r1=170029&r2=170028&pathrev=170029
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/url_request/url_request_unittest.cc?r1=170029&r2=170028&pathrev=170029
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome_frame/test/net/fake_external_tab.cc?r1=170029&r2=170028&pathrev=170029
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/url_request/url_request.cc?r1=170029&r2=170028&pathrev=170029
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/net/double_get_experiment_interceptor.cc?r1=170029&r2=170028&pathrev=170029
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/custom_handlers/protocol_handler_registry.cc?r1=170029&r2=170028&pathrev=170029

POSTs to HSTS domains are no longer converted to GETs 
on upgrade to https. 

URLRequestRedirectJobs now take a status code on construction, 
to reduce the change of regressions, and the status code can 
no longer be changed after construction. 

TBR=jhawkins@chromium.org 
BUG= 159343 

Review URL: https://chromiumcodereview.appspot.com/11420013
------------------------------------------------------------------------
Status: Fixed
Devtools displaying "GET" for these redirects is another matter - may look into it when I'm refactoring how devtools interfaces with the network stack.
Project Member Comment 19 by bugdroid1@chromium.org, Mar 11 2013
Labels: -Internals-Network -Internals-Network-HTTP Cr-Internals-Network-HTTP Cr-Internals-Network
Sign in to add a comment