Cookie: header may have an invalid value |
||||||||||||||
Issue descriptionAs part of Issue 627398 current dev builds crash when an invalid value is passed to SetHeader(). This resulted in crash 48eec1c100000000 with backtrace: Thread 14 CRASHED [EXCEPTION_ACCESS_VIOLATION_WRITE @ 0x00000000 ] MAGIC SIGNATURE THREAD 0x5ff81371 (chrome.dll -http_request_headers.cc:94 ) net::HttpRequestHeaders::SetHeader(base::BasicStringPiece<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > const &,base::BasicStringPiece<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > const &) 0x602e665e (chrome.dll -url_request_http_job.cc:824 ) net::URLRequestHttpJob::SetCookieHeaderAndStart(std::vector<net::CanonicalCookie,std::allocator<net::CanonicalCookie> > const &) 0x5ff63bc2 (chrome.dll -bind_internal.h:303 ) base::internal::InvokeHelper<1,void>::MakeItSo<void ( device_event_log::DeviceEventLogImpl::*const &)(device_event_log::DeviceEventLogImpl::LogEntry const &),base::WeakPtr<device_event_log::DeviceEventLogImpl> const &,device_event_log::DeviceEventLogImpl::LogEntry const &>(void ( device_event_log::DeviceEventLogImpl::*const &)(device_event_log::DeviceEventLogImpl::LogEntry const &),base::WeakPtr<device_event_log::DeviceEventLogImpl> const &,device_event_log::DeviceEventLogImpl::LogEntry const &) 0x601c517f (chrome.dll -bind_internal.h:324 ) base::internal::Invoker<base::internal::BindState<void ( `anonymous namespace'::ExtensionResourcesJob::*)(base::FilePath const &),base::WeakPtr<`anonymous namespace'::ExtensionResourcesJob> >,void >::Run(base::internal::BindStateBase *,base::FilePath const &) 0x602e5feb (chrome.dll -cookie_monster.cc:518 ) net::CookieMonster::GetCookieListWithOptionsTask::Run() 0x602e1757 (chrome.dll -cookie_monster.cc:2343 ) net::CookieMonster::DoCookieTaskForURL(scoped_refptr<net::CookieMonster::CookieMonsterTask> const &,GURL const &) 0x602e1477 (chrome.dll -cookie_monster.cc:915 ) net::CookieMonster::GetCookieListWithOptionsAsync(GURL const &,net::CookieOptions const &,base::Callback<void ,1> const &) 0x601a5365 (chrome.dll -url_request_http_job.cc:813 ) net::URLRequestHttpJob::AddCookieHeaderAndStart() 0x601a4e20 (chrome.dll -url_request_http_job.cc:398 ) net::URLRequestHttpJob::Start() 0x601a487d (chrome.dll -url_request.cc:659 ) net::URLRequest::StartJob(net::URLRequestJob *) 0x601a3e2b (chrome.dll -url_request.cc:602 ) net::URLRequest::BeforeRequestComplete(int) 0x601a3520 (chrome.dll -url_request.cc:528 ) net::URLRequest::Start() 0x601ff120 (chrome.dll -resource_loader.cc:497 ) content::ResourceLoader::StartRequestInternal() 0x601fc02d (chrome.dll -resource_loader.cc:187 ) content::ResourceLoader::StartRequest() 0x601fbc50 (chrome.dll -resource_dispatcher_host_impl.cc:2361 ) content::ResourceDispatcherHostImpl::StartLoading(content::ResourceRequestInfoImpl *,std::unique_ptr<content::ResourceLoader,std::default_delete<content::ResourceLoader> >) 0x601fb77a (chrome.dll -resource_dispatcher_host_impl.cc:2347 ) content::ResourceDispatcherHostImpl::BeginRequestInternal(std::unique_ptr<net::URLRequest,std::default_delete<net::URLRequest> >,std::unique_ptr<content::ResourceHandler,std::default_delete<content::ResourceHandler> >) 0x601f7e55 (chrome.dll -resource_dispatcher_host_impl.cc:1555 ) content::ResourceDispatcherHostImpl::BeginRequest(int,content::ResourceRequest const &,IPC::Message *,int) 0x601f74c3 (chrome.dll -resource_dispatcher_host_impl.cc:1166 ) content::ResourceDispatcherHostImpl::OnRequestResource(int,int,content::ResourceRequest const &) 0x601f73f8 (chrome.dll -ipc_message_templates.h:26 ) IPC::DispatchToMethod<content::ResourceDispatcherHostImpl,void ( content::ResourceDispatcherHostImpl::*)(int,int,content::ResourceRequest const &),void,std::tuple<int,int,content::ResourceRequest> >(content::ResourceDispatcherHostImpl *,void ( content::ResourceDispatcherHostImpl::*)(int,int,content::ResourceRequest const &),void *,std::tuple<int,int,content::ResourceRequest> const &) 0x601f6ccb (chrome.dll -ipc_message_templates.h:121 ) IPC::MessageT<ResourceHostMsg_RequestResource_Meta,std::tuple<int,int,content::ResourceRequest>,void>::Dispatch<content::ResourceDispatcherHostImpl,content::ResourceDispatcherHostImpl,void,void ( content::ResourceDispatcherHostImpl::*)(int,int,content::ResourceRequest const &)>(IPC::Message const *,content::ResourceDispatcherHostImpl *,content::ResourceDispatcherHostImpl *,void *,void ( content::ResourceDispatcherHostImpl::*)(int,int,content::ResourceRequest const &)) 0x601f6be6 (chrome.dll -resource_dispatcher_host_impl.cc:1108 ) content::ResourceDispatcherHostImpl::OnMessageReceived(IPC::Message const &,content::ResourceMessageFilter *) 0x601f6998 (chrome.dll -resource_message_filter.cc:46 ) content::ResourceMessageFilter::OnMessageReceived(IPC::Message const &) 0x601cec81 (chrome.dll -browser_message_filter.cc:70 ) content::BrowserMessageFilter::Internal::OnMessageReceived(IPC::Message const &) 0x601ceb0d (chrome.dll -message_filter_router.cc:22 ) IPC::`anonymous namespace'::TryFiltersImpl 0x601ceae2 (chrome.dll -message_filter_router.cc:87 ) IPC::MessageFilterRouter::TryFilters(IPC::Message const &) 0x601cea41 (chrome.dll -ipc_channel_proxy.cc:103 ) IPC::ChannelProxy::Context::TryFilters(IPC::Message const &) 0x601cea12 (chrome.dll -ipc_channel_proxy.cc:120 ) IPC::ChannelProxy::Context::OnMessageReceived(IPC::Message const &) 0x601ce9be (chrome.dll -ipc_channel_mojo.cc:394 ) IPC::ChannelMojo::OnMessageReceived(IPC::Message const &) 0x601ce755 (chrome.dll -ipc_message_pipe_reader.cc:152 ) IPC::internal::MessagePipeReader::Receive(mojo::Array<unsigned char>,mojo::Array<mojo::StructPtr<IPC::mojom::SerializedHandle> >) 0x601cd7e7 (chrome.dll -ipc.mojom.cc:693 ) IPC::mojom::ChannelStub::Accept(mojo::Message *) 0x601cd4b2 (chrome.dll -interface_endpoint_client.cc:311 ) mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message *) 0x601cd28f (chrome.dll -ipc.mojom.cc:779 ) IPC::mojom::ChannelRequestValidator::Accept(mojo::Message *) 0x601cd19f (chrome.dll -interface_endpoint_client.cc:262 ) mojo::InterfaceEndpointClient::HandleIncomingMessage(mojo::Message *) 0x601ccf17 (chrome.dll -ipc_mojo_bootstrap.cc:645 ) IPC::`anonymous namespace'::ChannelAssociatedGroupController::Accept 0x6018f487 (chrome.dll -message_header_validator.cc:93 ) mojo::MessageHeaderValidator::Accept(mojo::Message *) 0x6018eac7 (chrome.dll -connector.cc:275 ) mojo::Connector::ReadSingleMessage(unsigned int *) 0x6018ea4b (chrome.dll -connector.cc:302 ) mojo::Connector::ReadAllAvailableMessages() 0x6018ea31 (chrome.dll -connector.cc:234 ) mojo::Connector::OnHandleReadyInternal(unsigned int) 0x5ff5da58 (chrome.dll -bind_internal.h:324 ) base::internal::Invoker<base::internal::BindState<void ( `anonymous namespace'::HttpRequest::*)(int),base::internal::UnretainedWrapper<`anonymous namespace'::HttpRequest> >,void >::Run(base::internal::BindStateBase *,int &&) 0x6018e9ee (chrome.dll -watcher.cc:122 ) mojo::Watcher::OnHandleReady(unsigned int) 0x6018e99a (chrome.dll -bind_internal.h:303 ) base::internal::InvokeHelper<1,void>::MakeItSo<void ( net::URLRequestSimpleJob::*const &)(int),base::WeakPtr<net::URLRequestSimpleJob> const &,int>(void ( net::URLRequestSimpleJob::*const &)(int),base::WeakPtr<net::URLRequestSimpleJob> const &,int &&) 0x6018e96f (chrome.dll -bind_internal.h:324 ) base::internal::Invoker<base::internal::BindState<void ( net::URLRequestJob::*)(int),base::WeakPtr<net::URLRequestSimpleJob>,int>,void >::Run(base::internal::BindStateBase *) 0x5fe70424 (chrome.dll -task_annotator.cc:54 ) base::debug::TaskAnnotator::RunTask(char const *,base::PendingTask const &) 0x5fe701dc (chrome.dll -message_loop.cc:496 ) base::MessageLoop::RunTask(base::PendingTask const &) 0x5fe6fbd9 (chrome.dll -message_loop.cc:629 ) base::MessageLoop::DoWork() 0x5fe6f727 (chrome.dll -message_pump_win.cc:727 ) base::MessagePumpForIO::DoRunLoop() 0x5fe6f64f (chrome.dll -message_pump_win.cc:140 ) base::MessagePumpWin::Run(base::MessagePump::Delegate *) 0x5fe6f447 (chrome.dll -run_loop.cc:35 ) base::RunLoop::Run() 0x5fe6f3f8 (chrome.dll -thread.cc:228 ) base::Thread::Run(base::RunLoop *) 0x5ff548b6 (chrome.dll -browser_thread_impl.cc:243 ) content::BrowserThreadImpl::IOThreadRun(base::RunLoop *) 0x5ff2641e (chrome.dll -browser_thread_impl.cc:278 ) content::BrowserThreadImpl::Run(base::RunLoop *) 0x5fe6de8e (chrome.dll -thread.cc:301 ) base::Thread::ThreadMain() 0x5fe6dba6 (chrome.dll -platform_thread_win.cc:84 ) base::`anonymous namespace'::ThreadFunc 0x76b8ee1b (kernel32.dll + 0x0004ee1b ) BaseThreadInitThunk 0x76f737ea (ntdll.dll + 0x000637ea ) __RtlUserThreadStart 0x76f737bd (ntdll.dll + 0x000637bd ) _RtlUserThreadStart It appears that net::ParsedCookie takes the principle of "be liberal in what you accept" very seriously. Unfortunately, net::CookieStore::BuildCookieLine() fails to be as serious about "be conservative in what you send". Making cookie parsing stricter seems the most attractive option, but also the most likely to break the web. Modifying CookieStore::BuildCookieLine to check each value against HttpUtil::IsValidHeaderValue() is more minimalist but may have performance impact. Leaving things as they are is another option, but there may be a legitimate security issue lurking here.
,
Aug 16 2016
,
Aug 16 2016
,
Aug 16 2016
Joel, is this something you can poke at along with the other parser changes you're looking into?
,
Aug 16 2016
We may well be stuck sending invalid cookie headers, if that's what other browsers do.
,
Aug 16 2016
The CHECK is just for cookie values with '\0', '\r', or '\n's in them. The latter two should only be possible if something external to a site is setting them. Not sure if that's also the case for '\0'. Regardless, I think we're fairly safe not sending cookies with those in their values (Or in their names, for that matter). Would be good if we could figure out where those are coming from: Corrupted cookie databases, corrupted memory, chrome code, extensions, or if sites are actually managing to set such cookies.
,
Aug 17 2016
On further inspection, none of "\0", "\r" or "\n" can be set via HTTP headers, because net::HttpUtil::AssembleRawHeaders splits on "\r" and "\n" and removes "\0" from the input. https://cs.chromium.org/chromium/src/net/http/http_util.cc?q=AssembleRawHeaders Assigning via document.cookie also doesn't work; anything after "\0", "\r" or "\n" is ignored. There may be other ways for cookies to end up in the cookie store, or maybe this crash is entirely due to memory or disk corruption I still would prefer to add an IsValidHeaderValue() check to CookieStore::BuildCookieLine() for if/when we make the check stricter in future. A quick check of other browsers indicates that Set-Cookie: a=embedded\rcr Results in "a=embedded" in Chrome and IE, and "a=embedded; cr" in Firefox.
,
Aug 17 2016
Curiously, we also have "IsValidCookieValue", which looks to only be called when extensions set cookies, but not when they're set by remote sites (?). It also disallows those characters, so extensions can't set those values in cookies either. We really should think about unifying logic for what cookie values we allow/don't allow.
,
Aug 17 2016
...and then we also have IsValidCookieAttributeValue, which is what we actually use during cookie line parsing. I'm surprised that SetValue doesn't use a combination of that and IsValidToken instead of rolling it's own validator. As a path froward, at the very least, we probably ought to write some unit tests that verify some ordering of these functions so we know and assure which is a superset of which. As ricea@ mentioned in #7, I think the other browsers do indeed generally accept \r, which agrees with http://inikulin.github.io/cookie-compat/#DISABLED_CHROMIUM0023 (although Safari seems to reject it).
,
Aug 17 2016
Our header parser currently treats "\n", "\r\n", and "\r" as header line breaks, I believe, so we can't accept "\r" in set-cookie headers without changing that.
,
Aug 24 2016
I took a another look at net::ParsedCookie and there are a couple of things I consider bogus: * ParsedCookie::SetValue() checks IsValidCookieValue() but ParseTokenValuePairs() (and so the constructor) doesn't. * ParsedCookie::IsValid() doesn't check IsValidCookieValue(). Maybe the assumption is that it must already have already been checked?
,
Aug 24 2016
Temporarily taking this bug to add a histogram.
,
Aug 24 2016
I misread the code. ParseTokenValuePairs() validates the cookie against IsValidCookieAttribute(). This still is inconsistent with SetValue(), which performs a stronger check. I am going to add a histogram anyway. If usage of invalid cookie values is rare we may be able to make the behaviour more consistent.
,
Aug 24 2016
Prior work: Issue 238041 investigated the same issues and excluded control characters from the settable values jww@ on that bug pointed out that we have compatibility tests that would be broken by being stricter in handling backslash and quote characters.
,
Aug 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b08ddda70e3f9c261d1fb87a2e63fbc14f9b8775 commit b08ddda70e3f9c261d1fb87a2e63fbc14f9b8775 Author: ricea <ricea@chromium.org> Date: Fri Aug 26 12:27:35 2016 Add Cookie.CookieLineCookieValueValidity histogram net::ParsedCookie::SetValue() applies a strict check to the passed-in value, but ParsedCookie::ParseTokenValuePairs() applies a weaker check. It would nice if this was consistent. Add a histogram to determine the degree of compatibility problems with making it consistent. BUG=638117 Review-Url: https://codereview.chromium.org/2273633004 Cr-Commit-Position: refs/heads/master@{#414690} [modify] https://crrev.com/b08ddda70e3f9c261d1fb87a2e63fbc14f9b8775/net/cookies/parsed_cookie.cc [modify] https://crrev.com/b08ddda70e3f9c261d1fb87a2e63fbc14f9b8775/tools/metrics/histograms/histograms.xml
,
Sep 5 2016
The above CL missed the branch for M54, so it will be a long time before we have data from stable. Having said that, canary data indicates that 1.4% of cookies fail the stricter check. Even on low volume, that looks pretty bad.
,
Sep 26 2016
Issue 650149 has been merged into this issue.
,
Sep 26 2016
Users experienced this crash on the following builds: Win Dev 55.0.2868.3 - 0.23 CPM, 52 reports, 49 clients (signature free) Win Beta 54.0.2840.34 - 0.21 CPM, 128 reports, 120 clients (signature free) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Jul 14 2017
Results of Cookie.CookieLineCookieValueValidity (stable channel, over the last 14 days): Invalid 03.50% Valid 96.50% There's no way we could enforce the tighter restriction in the foreseeable future. I wonder if we should just loosen the restriction in the SetCookie() method instead. I noticed that the check performed by IsValidCookieValue() is based on the syntax in section 4.1.1 of RFC 6265, but the algorithm from section 5.2 accepts a much larger set of strings.
,
Jul 14 2017
[+rdsmith]: Who's working on a cookie mojo interface, and added some cookie validation logic as a result (We used to rely on the cookie line to canonical cookie conversion to enforce at least some form of validity, but the Mojo API will take CanonicalCookies as input).
,
Jul 14 2017
Reading a bit more of RFC 6265 I found that section 4.2.1 explains why we have two different notions of validity most clearly: The user agent sends stored cookies to the origin server in the Cookie header. If the server conforms to the requirements in Section 4.1 (and the user agent conforms to the requirements in Section 5), the user agent will send a Cookie header that conforms to the [...] grammar[...] This doesn't take into account that syntactically invalid cookies can be set by other means (ie. Javascript) but that's probably out of scope for the RFC anyway. To sum up the whole issue: invalid cookies can lead us to transmitting invalid HTTP headers. Because we disallow NUL, CR and LF this hopefully has no security implications. I think we probably have adequate tests to ensure we keep preventing NUL, CR and LF from finding their way into cookies, but I haven't looked recently. Invalid cookies are common and we can't reasonably block them so I think we have to accept the status quo. The one remaining thing we might do here is make SetCookie() more liberal.
,
Jul 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01dfc1dcc85288393054c326647d1b7b88420cf4 commit 01dfc1dcc85288393054c326647d1b7b88420cf4 Author: Adam Rice <ricea@chromium.org> Date: Fri Jul 14 23:36:27 2017 Remove Cookie.CookieLineCookieValueValidity histogram We have collected adequate data from Cookie.CookieLineCookieValueValidity. Remove it. BUG=638117 Change-Id: Iab9d2b095cebda9a4b1cf0fdb572aa6ee30a457d Reviewed-on: https://chromium-review.googlesource.com/571884 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Commit-Queue: Ilya Sherman <isherman@chromium.org> Cr-Commit-Position: refs/heads/master@{#486939} [modify] https://crrev.com/01dfc1dcc85288393054c326647d1b7b88420cf4/net/cookies/parsed_cookie.cc [modify] https://crrev.com/01dfc1dcc85288393054c326647d1b7b88420cf4/tools/metrics/histograms/histograms.xml
,
Feb 16 2018
,
Mar 4 2018
Users experienced this crash on the following builds: Mac Canary 67.0.3360.0 - 0.30 CPM, 1 reports, 1 clients (signature free) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
May 16 2018
,
May 23 2018
,
Jul 6
,
Jul 6
|
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by ricea@chromium.org
, Aug 16 2016