Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rgw: sanitize the HTTP_* http header elements #29814

Closed
wants to merge 1 commit into from
Closed

rgw: sanitize the HTTP_* http header elements #29814

wants to merge 1 commit into from

Conversation

mkogan1
Copy link
Contributor

@mkogan1 mkogan1 commented Aug 22, 2019 •
edited

to remove trailing <CR> and <LF> characters
which can cause swift requests to fail authentication
when present in the HTTP_X_AUTH_TOKEN

in addition, will sanitize also the following headers for example:
HTTP_HOST
HTTP_USER_AGENT
HTTP_ACCEPT

Fixes: https://tracker.ceph.com/issues/41376

Signed-off-by: Mark Kogan mkogan@redhat.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test docs
  • jenkins render docs

@mkogan1 mkogan1 self-assigned this Aug 22, 2019
mattbenjamin
mattbenjamin reviewed Aug 22, 2019
View reviewed changes
src/rgw/rgw_civetweb.cc Outdated
@@ -129,7 +129,14 @@ int RGWCivetWeb::init_env(CephContext *cct)
}
*dest = '\0';

env.set(buf, value);
string sanitized_value = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

this works, but should we be considering the entire header? I mean, the reported issue is just extra data at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, pushed a change that moves the sanitization to top of the header elements processing loop to cover all the header elements.

@mkogan1
rgw: sanitize the HTTP_* http header elements
f8295db 
to remove trailing <CR> and <LF> characters
which can cause swift requests to fail authentication
when present in the HTTP_X_AUTH_TOKEN

in addition will clean also the following headers for example:
HTTP_HOST
HTTP_USER_AGENT
HTTP_ACCEPT

Fixes: https://tracker.ceph.com/issues/41376

Signed-off-by: Mark Kogan <mkogan@redhat.com>
@stale
Copy link

stale bot commented Oct 21, 2019

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Oct 21, 2019
@mkogan1
Copy link
Contributor Author

mkogan1 commented Oct 24, 2019

please unstale....

@stale stale bot removed the stale label Oct 24, 2019
@cbodley
Copy link
Contributor

cbodley commented Nov 14, 2019

@mkogan1 what else needs to be done here?

@mkogan1
Copy link
Contributor Author

mkogan1 commented Nov 19, 2019

@cbodley will add a ceph.conf option to disable by default

yuvalif
yuvalif reviewed Nov 19, 2019
View reviewed changes
src/rgw/rgw_civetweb.cc
@@ -101,7 +101,13 @@ int RGWCivetWeb::init_env(CephContext *cct)
}

const boost::string_ref name(header->name);
const auto& value = header->value;
string value = header->value;
Copy link
Contributor

Choose a reason for hiding this comment

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

why copy to "value"?
can you modify in place (use: auto& value = header->value)?

@cbodley
Copy link
Contributor

cbodley commented Jan 16, 2020

ping @mkogan1

@cbodley
Copy link
Contributor

cbodley commented Jan 16, 2020

after further discussion, since this can only apply to civetweb, i don't think it's worth adding this workaround for buggy clients

@cbodley cbodley closed this Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattbenjamin mattbenjamin mattbenjamin left review comments

@yuvalif yuvalif yuvalif left review comments

@cbodley cbodley Awaiting requested review from cbodley

Assignees

@mkogan1 mkogan1

Labels
bug-fix DNM rgw
Projects
None yet
Milestone
No milestone
4 participants
@mkogan1 @cbodley @mattbenjamin @yuvalif