Swift tempurl middleware reveals signatures in the logfiles (CVE-2017-8761)

Bug #1685798 reported by Christian Schwede
256
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Fix Released
Undecided
Christian Schwede
OpenStack Security Advisory
Won't Fix
Undecided
Unassigned
Swift3
New
Undecided
Unassigned

Bug Description

The proxy server will log valid temporary urls, that might be used to gain access to data by anyone with access to the logfiles. This is especially important with tempurls that are valid for extended
periods and/or when using central logging servers, accessed by operators that have no access to the Swift servers.

Example logentry:

 Apr 24 13:25:16 localhost proxy-server[5849]: 127.0.0.1 127.0.0.1 24/Apr/2017/13/25/16 GET /v1/AUTH_test/test/something%3Ftemp_url_sig%3D99e80d557807904d15c69f4ef85bce42cfcd0bd5%26temp_url_expires%3D1493041071 HTTP/1.0 401 - curl/7.51.0 - - 35 - tx01de01a8da5a4052988bc-0058fdfcbc - 0.0178 - - 1493040316.148339987 1493040316.166150093 -

I propose to trim the temp_url_sig, like we are already doing for tokens - see attached patch.

CVE References

Revision history for this message
Christian Schwede (cschwede) wrote :
Revision history for this message
Donagh McCabe (donagh-mccabe) wrote :

Having had to debug customer reports that tempurl "does not work", I found it can be be useful to see exactly what they sent, vs what they thought they sent. So as debug aid, I suggest you have a way of switching this off with something like:

    [tempurl]
    log_signatures_for_accounts: AUTH_test,AUTH_other

By only logging for specific accounts, you don't expose all users. You can advise the account owner to change their keys after the debug period is over.

[Even with logging, it can still be hard to debug because the proxy-logger URL-encodes before logging and this is often the area where signature-encoding is wrong to start with]

Revision history for this message
Tim Burke (1-tim-z) wrote :

Fairly certain there are similar concerns for swift3 and pre-signed urls. See http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html for what those look like.

Revision history for this message
clayg (clay-gerrard) wrote :

+2 WFM

Apr 24 18:53:14 ubuntu-xenial object-6010: 127.0.0.1 - - [24/Apr/2017:18:53:14 +0000] "HEAD /sdb1/645/AUTH_test/test/test" 200 8 "HEAD http://saio:8080/v1/AUTH_test/test/test?temp_url_expires=1493063464&temp_url_sig=d548ccbc..." "tx7decc83acfd9426bb3a10-0058fe499a" "proxy-server 10740" 0.0006 "-" 10735 0

I think Donagh's point is spot on

    [filter:tempurl]
    use = egg:swift#tempurl
    reveal_sensitive_prefix = 99999

^ that works for me, we definitely need to follow up with that in the example file.

Donagh, I think exempting accounts from truncation would be a good feature for followup after we get out from under the embargo. I can also confirmed, as Christian's commit message points out, that only authorized signatures are truncated - which could help with debugging for ops:

Apr 24 18:59:59 ubuntu-xenial proxy-server: 127.0.0.1 127.0.0.1 24/Apr/2017/18/59/59 HEAD /v1/AUTH_test/test/test%3Ftemp_url_sig%3Dbf2431dd9860764fb423a08d3bf7d01cd4312004%26temp_url_expires%3D1493063999 HTTP/1.0 401 - curl/7.47.0 - - - - tx3d9ba159317e45ee87c05-0058fe4b2f - 0.0016 - - 1493060399.179332018 1493060399.180957079 -

... or eventlet debug

Apr 24 19:00:08 ubuntu-xenial proxy-server: 127.0.0.1 127.0.0.1 24/Apr/2017/19/00/08 HEAD /v1/AUTH_test/test/test%3Ftemp_url_expires%3D1493064008%26temp_url_sig%3D3cafada5... HTTP/1.0 200 - curl/7.47.0 - - - - tx2db8598376d14dc2a48eb-0058fe4b38 - 0.0274 - - 1493060408.930609941 1493060408.957966089 0
Apr 24 19:00:08 ubuntu-xenial proxy-server: STDERR: 127.0.0.1 - - [24/Apr/2017 19:00:08] "HEAD /v1/AUTH_test/test/test?temp_url_sig=3cafada56bcdf1a37176926c13199e4251a4e010&temp_url_expires=1493064008 HTTP/1.1" 200 475 0.029963 (txn: tx2db8598376d14dc2a48eb-0058fe4b38)

So I think we have Donagh's very correct concern satiated and can/should move forward here to get out from embargo.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

Changed in ossa:
status: New → Incomplete
description: updated
Revision history for this message
John Dickinson (notmyname) wrote :

The patch works. I have a couple of small nits: (1) the example conf and associated docs weren't updated and (2) no matter the prefix length given, the logged value will now always end with "..."

I'm fine with (2) being handled in a follow-up, but I think it's a bad idea to land a patch and wait for docs later.

Revision history for this message
Tim Burke (1-tim-z) wrote :

-1

The short-circuiting around OPTIONS requests [1] makes me nervous. If you've got a web app that uses tempurls, won't we still log the sig in the CORS pre-flight request? Similar concerns around the check for disallowed headers [2] -- and at that point, we *know* the sig is valid.

[1] https://github.com/openstack/swift/blob/2.13.0/swift/common/middleware/tempurl.py#L395-L396
[2] https://github.com/openstack/swift/blob/2.13.0/swift/common/middleware/tempurl.py#L445

Revision history for this message
Tim Burke (1-tim-z) wrote :

This should take care of swift3. I'll start looking to address my comments for Christian's patch, but I'm not sure I'll have something to post tonight.

Revision history for this message
Tim Burke (1-tim-z) wrote :

I think this is an improvement? Apply on top of Christian's patch.

Revision history for this message
Christian Schwede (cschwede) wrote :

Thanks everyone for your input!

I like Donagh's idea to allow disabling this for specific accounts; will will submit a public proposal once this landed.

I stashed Tim's patch into mine, and also updated the sample and docstring and added one more test.

Revision history for this message
Tim Burke (1-tim-z) wrote :

+2 on the new patch; looks good to me.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Please review the proposed impact description for a Swift advisory (assuming the solution in the attached patches will also work for supported stable/newton and stable/ocata branches), and let me know what needs to be clarified or corrected:

Title: Swift proxy-server logs tempurl signatures
Reporter: Christian Schwede (Red Hat)
Products: Swift
Affects: <=2.10.1, >=2.11.0 <=2.13.0

Description:
Christian Schwede with Red Hat reported a vulnerability in Swift. The proxy-server logs full tempurl paths, potentially leaking reusable tempurl signatures to anyone with read access to these logs. All Swift deployments using the tempurl middleware are affected.

Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

The impact description looks good to me.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Swift reviewers, does the proposed impact description in comment #12 basically cover the risk/exposure here? And are there plans to backport the fix to stable/newton and stable/ocata branches? If yes on all counts, I'll go ahead with requesting a CVE assignment from MITRE.

Revision history for this message
John Dickinson (notmyname) wrote :

I think the description is accurate. Once we have the backports available, we'll land those as well.

I'd like to have Christian's take on the CVE description. Also, I'd like Tim's take on the related swift3 impact, so a reveal here doesn't also reveal a bug in that project.

Revision history for this message
Christian Schwede (cschwede) wrote :

If possible, I would like to mention the original reporter of this in the CVE:

"Bülent Topcu from Turkcell reported a vulnerability in Swift."

He reported this to me, is aware of the upcoming CVE and fine being mentioned in the CVE itself. He is not on Launchpad, therefore I can't add him directly as a subscriber to this CVE.

Description itself looks good to me. Backport for Newton has a small conflict, but nothing serious; Ocata backport should be doable in Gerrit itself.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Thanks John and Christian! So sounds like I can privately request a CVE assignment using the revised impact description below (with reporter details corrected and taking last week's 2.14.0 release on master into account), and we can hold off scheduling publication of the advisory and patches until we hear back from Tim:

Title: Swift proxy-server logs tempurl signatures
Reporter: Bülent Topcu (Turkcell)
Products: Swift
Affects: <=2.10.1, >=2.11.0 <=2.13.0, ==2.14.0

Description:
Bülent Topcu with Turkcell reported a vulnerability in Swift. The proxy-server logs full tempurl paths, potentially leaking reusable tempurl signatures to anyone with read access to these logs. All Swift deployments using the tempurl middleware are affected.

Jeremy Stanley (fungi)
Changed in ossa:
status: Incomplete → Triaged
importance: Undecided → Medium
assignee: nobody → Jeremy Stanley (fungi)
Jeremy Stanley (fungi)
Changed in ossa:
status: Triaged → In Progress
Jeremy Stanley (fungi)
summary: Swift tempurl middleware reveals signatures in the logfiles
+ (CVE-2017-8761)
Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

The new affect line is: >=2.11.0 <=2.13.1, >=2.14.0 <=2.15.1 (assuming eoled newton)

Revision history for this message
Tim Burke (1-tim-z) wrote :

Yeah, that all looks sane. It may be worth noting that pre-signed URLs for Swift3 are similarly affected -- the mechanism is very similar to Swift tempurls. With the current plans to reintegrate Swift3 into Swift, it seems like patching Swift3 will be a pretty low priority.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Looks like this report fell through the cracks back in 2017 when the VMT was waiting for feedback. Is it still relevant now, and should we put it back in the pipeline to patch/announce?

description: updated
Revision history for this message
Jeremy Stanley (fungi) wrote :

In keeping with recent OpenStack vulnerability management policy changes, no report should remain under private embargo for more than 90 days. Because this report predates the change in policy, the deadline for public disclosure is being set to 90 days from today. If the report is not resolved within the next 90 days, it will revert to our public workflow as of 2020-05-27. Please see http://lists.openstack.org/pipermail/openstack-discuss/2020-February/012721.html for further details.

Revision history for this message
Jeremy Stanley (fungi) wrote :

It doesn't look like this report has seen any activity since my update two months ago, so consider this a friendly reminder:

The embargo for this report is due to expire one month from today, on May 27, and will be switched public on or shortly after that day if it is not already resolved sooner.

Thanks!

Jeremy Stanley (fungi)
description: updated
Revision history for this message
Jeremy Stanley (fungi) wrote :

The embargo for this report has expired and is now lifted, so it's acceptable to discuss further in public.

description: updated
information type: Private Security → Public Security
Changed in ossa:
status: In Progress → Incomplete
assignee: Jeremy Stanley (fungi) → nobody
importance: Medium → Undecided
Revision history for this message
clayg (clay-gerrard) wrote :
Changed in swift:
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (master)

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/swift/+/822585

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (master)

Reviewed: https://review.opendev.org/c/openstack/swift/+/822585
Committed: https://opendev.org/openstack/swift/commit/f2c279bae94689e2062beb6d0030d168a0b4cbdf
Submitter: "Zuul (22348)"
Branch: master

commit f2c279bae94689e2062beb6d0030d168a0b4cbdf
Author: Matthew Oliver <email address hidden>
Date: Thu Feb 3 16:29:53 2022 +1100

    Trim sensitive information in the logs (CVE-2017-8761)

    Several headers and query params were previously revealed in logs but
    are now redacted:

      * X-Auth-Token header (previously redacted in the {auth_token} field,
        but not the {headers} field)
      * temp_url_sig query param (used by tempurl middleware)
      * Authorization header and X-Amz-Signature and Signature query
        parameters (used by s3api middleware)

    This patch adds some new middleware helper methods to track headers and
    query parameters that should be redacted by proxy-logging. While
    instantiating the middleware, authors can call either:

       register_sensitive_header('case-insensitive-header-name')
       register_sensitive_param('case-sensitive-query-param-name')

    to add items that should be redacted. The redaction uses proxy-logging's
    existing reveal_sensitive_prefix config option to determine how much to
    reveal.

    Note that query params will still be logged in their entirety if
    eventlet_debug is enabled.

    UpgradeImpact
    =============
    The reveal_sensitive_prefix config option now applies to more items;
    operators should review their currently-configured value to ensure it
    is appropriate for these new contexts. In particular, operators should
    consider reducing the value if it is more than 20 or so, even if that
    previously offered sufficient protection for auth tokens.

    Co-Authored-By: Tim Burke <email address hidden>
    Closes-Bug: #1685798
    Change-Id: I88b8cfd30292325e0870029058da6fb38026ae1a

Changed in swift:
status: In Progress → Fix Released
Revision history for this message
Jeremy Stanley (fungi) wrote :

I'm thrilled to see that a fix finally merged in master for this, thanks for driving it to a conclusion!

Given that the bug report is 5 years old, and has been public for nearly 2 years now, I suspect there's not a lot of point in issuing a security advisory for it. If there's interest though, and a plan to backport the fix to maintained stable branches, please reply and I'll switch our OSSA bugtask back to an active state.

Changed in ossa:
status: Incomplete → Won't Fix
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on swift (master)

Change abandoned by "Tim Burke <email address hidden>" on branch: master
Review: https://review.opendev.org/c/openstack/swift/+/817476
Reason: Took a different approach: https://review.opendev.org/c/openstack/swift/+/826947

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/swift 2.29.0

This issue was fixed in the openstack/swift 2.29.0 release.

To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers