Limit on preserved POST data size is not enforced

Description

From reading the code for AbstractHandler::getPostData, I found that the postLimit setting can be used to limit the allowed size of initial application/x-www-form-urlencoded POST data to be preserved.

However, actually testing this out shows that this limit is not actually being enforced. The config from shibboleth2.xml and commands I used to test are:

<Sessions lifetime="14400" timeout="300" checkAddress="true" idpHistory="false" handlerURL="/.sso" handlerSSL="true" cookieProps="; path=/; secure; HttpOnly" relayState="ss:mem" redirectLimit="exact" postData="ss:mem" postLimit="512000">
$ perl -e 'print "a=".("a"x(512001-2));' >postdata $ curl -k --data @postdata --proxy "" https://myserver/

According to the code, if the submitted POST data exceeds the limit, a warning should be logged indicating that POST data was lost, but I did not find any such log entry:

AbstractHandler.cpp

DDF AbstractHandler::getPostData(const Application& application, const HTTPRequest& request) const { string contentType = request.getContentType(); if (contentType.find("application/x-www-form-urlencoded") != string::npos) { const PropertySet* props = application.getPropertySet("Sessions"); pair<bool,unsigned int> plimit = props ? props->getUnsignedInt("postLimit") : pair<bool,unsigned int>(false,0); if (!plimit.first) plimit.second = 1024 * 1024; request.getRequestBody(); if (plimit.second == 0 || request.getContentLength() <= plimit.second) { CGIParser cgi(request); pair<CGIParser::walker,CGIParser::walker> params = cgi.getParameters(nullptr); if (params.first == params.second) return DDF("parameters").list(); DDF child; DDF ret = DDF("parameters").list(); for (; params.first != params.second; ++params.first) { if (!params.first->first.empty()) { child = DDF(params.first->first.c_str()).unsafe_string(params.first->second); ret.add(child); } } return ret; } else { m_log.warn("POST limit exceeded, ignoring %d bytes of posted data", request.getContentLength()); } } else { m_log.info("ignoring POST data with non-standard encoding (%s)", contentType.c_str()); } return DDF(); }

Stepping through the code in a debugger shows that the call to request.getContentLength() always returns 0. For my Apache environment, this method is implemented by ShibTargetApache::getContentLength:

mod_shib.cpp

long getContentLength() const { return m_gotBody ? m_body.length() : m_req->remaining; }

The m_gotBody field is always false, even for POST requests with a request body. It looks like this field only becomes initialized after a call to ShibTargetApache::getRequestBody(), but setting a breakpoint on this function shows that it is never called before the getContentLength() check.

I have to admit that I don't know why m_req->remaining would return 0 for POSTs with a request body, though. Given that the request body was not yet read by us in getRequestBody(), I would expect this field to contain the POST request size rather than 0.

Either way, the getRequestBody() call is first made as part of the CGIParser stuff to fetch the POST parameters, which enables it to get the correct values, but unfortunately the POST data size check has already been passed at that point.

As a quick check to verify all of the above, I recompiled with the following modification to AbstractHandler::getPostData:

AbstractHandler.cpp

// ... if (!plimit.first) plimit.second = 1024 * 1024; request.getRequestBody(); if (plimit.second == 0 || request.getContentLength() <= plimit.second) { CGIParser cgi(request); // ...

Indeed, this modification ensures that the m_gotBody field is initialized, and enables the POST size check to work. (Note that I'm not advocating the above modification as a solution, it's merely a technical test to see if initializing the field would allow the check to work).

Finally, the postLimit setting does not seem to be documented. I was been looking for this functionality in the docs before I found it in the code, so I imagine other people might be too.

Environment

Red Hat Enterprise Linux Server release 5.8 (Tikanga)
Apache 2.2.23

Activity

Scott Cantor December 2, 2013 at 5:06 PM

Closing on release.

Scott Cantor November 21, 2013 at 6:44 PM

myhandisadolphin@mailinator.com July 4, 2013 at 1:20 PM

Well, I accidentally listed the extra request.getRequestBody() that I added for testing in the original source code listing for AbstractHandler::getPostData. I can no longer edit the description text though, so ignore that :o)

Fixed

Assignee

Reporter

Components

Fix versions

Affects versions

Created July 4, 2013 at 1:15 PM
Updated December 2, 2013 at 5:06 PM
Resolved November 21, 2013 at 6:44 PM