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

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]

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.

+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.

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.

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.

-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

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.

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

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.

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

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.

The impact description looks good to me.

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.

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.

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.

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.

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

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.

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?

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.

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!

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

https://review.opendev.org/c/openstack/swift/+/817476

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

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

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.

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

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