Skip to content
This repository has been archived by the owner on Jan 28, 2020. It is now read-only.

Handle ap_get_client_block() error in am_read_post_data() #71

Merged
merged 1 commit into from Mar 2, 2016

Conversation

vrasneur
Copy link

@vrasneur vrasneur commented Mar 2, 2016

Hello,

I had the same segmentation fault as in the issue #48.

Here is a patch that should fix the segfault: am_read_post_data() should check if ap_get_client_block() returns an error.

The patch also fixes a possible underflow in am_read_post_data() if mod_auth_mellon receives too much POST data. I have not checked if it is possible to trigger that underflow.

Regards,

Vincent Rasneur
Software security engineer, DenyAll
vrasneur@denyall.com

ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
"Failed to read POST data from client.");
return HTTP_INTERNAL_SERVER_ERROR;
}
else if ((apr_size_t)read_length > bytes_left) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify this a bit -- in what cases will ap_get_client_block return more than bytes_left?

From the documentation, it looks like it shouldn't do that:

/**
 * Call this in a loop.  It will put data into a buffer and return the length
 * of the input block
 * @param r The current request
 * @param buffer The buffer in which to store the data
 * @param bufsiz The size of the buffer
 * @return Number of bytes inserted into the buffer.  When done reading, 0
 *         if EOF, or -1 if there was an error
 */

@olavmrk
Copy link
Contributor

olavmrk commented Mar 2, 2016

Thanks for looking into this!

You are right that we need to handle errors condition from ap_get_client_block(). The failure mode is a bit interesting -- as far as I can tell, we will repeatedly call ap_get_client_block() with an ever-larger read request. Not sure how many iterations are required before it crashes though.

I do strongly suspect that it crashes on the second iteration, since at that point we are asking Apache to read from something that has already returned an error.

Looking at the code, I think we also need to put an upper bound to the number of bytes that we are willing to read, but I think that belongs in a separate patch.

*/
read_length = ap_get_client_block(r, &(*data)[bytes_read],
bytes_left);
if (read_length == 0) {
// got the EOF
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that getting an "early" EOF (before we get the expected number of bytes) isn't an error, I think the code needs to adjust a couple of things:

  • Add a null terminator (like is done at line 570).
  • Update length with the actual number of bytes read (as on line 564).

@vrasneur
Copy link
Author

vrasneur commented Mar 2, 2016

Hi olavmrk,

As I have said, I did not check if the underflow is triggerable. The main fix is to check for the ap_get_client_block() error.

For the crash, when ap_get_client_block() returns -1, the loop in mod_auth_mellon will allocate an "infinite" number of empty bucket brigades.

On my coredump, you could see that Apache had created approximately 451,000,000 empty brigades before crashing ;-)

@olavmrk
Copy link
Contributor

olavmrk commented Mar 2, 2016

As I have said, I did not check if the underflow is triggerable. The main fix is to check for the ap_get_client_block() error.

I don't think it can ever happen (ap_get_client_block() returning more data than requested) , so I'd remove that condition. Leaving it in will only lead to confusion for future generations.

For the crash, when ap_get_client_block() returns -1, the loop in mod_auth_mellon will allocate an "infinite" number of empty bucket brigades.

OK, that is good to know. Then this crashes the web server process, but doesn't allow for overwriting arbitrary data.

Could you amend your patch with:

  • The check for (apr_size_t)read_length > bytes_left removed.
  • Adjustments of length and null terminator on early EOF.

@vrasneur
Copy link
Author

vrasneur commented Mar 2, 2016

I have updated the patch.

Is it OK for you?

olavmrk added a commit that referenced this pull request Mar 2, 2016
Handle ap_get_client_block() error in am_read_post_data()
@olavmrk olavmrk merged commit ccb7dc3 into Uninett:master Mar 2, 2016
@olavmrk
Copy link
Contributor

olavmrk commented Mar 2, 2016

Yes, thanks!

jhrozek added a commit to jhrozek/mod_auth_mellon that referenced this pull request Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants