search

go-resty / resty

Simple HTTP and REST client library for Go
MIT License
9.98k stars 706 forks source link

Security: Don't put the same bytes.Buffer into sync.Pool twice #745

Closed lattwood closed 9 months ago

lattwood commented 11 months ago

This fixes #743, possibly #739.

Discovered this while tracking down an issue in https://github.com/linode/terraform-provider-linode.

The removed call to sync.Pool.Put is handled by the request body wrapper that puts the buffer back in the pool when the body is closed.

lattwood commented 11 months ago

This closes #739 as well.

Now that I’m thinking about it, this is a security issue, as it’s disclosing a HTTP body to an unrelated endpoints/servers.

reedloden commented 10 months ago

@jeevatkm Able to take a look here when you have a moment?

jxsl13 commented 10 months ago

@jeevatkm pls merge

safaci2000 commented 10 months ago

Any updates on this ticket?

lattwood commented 10 months ago

Your best bet is going to be forking this, or moving to a different library, based on how long this issue has been open.

niksteff commented 10 months ago

Yeah we are moving. Not only is the CVE a big problem, after the update to 2.10 we experienced fun bugs where the json request body of a previous or the same request (could not tell) was copied in the host part of the request URL. This occurred when multiple request were running concurrently and we changed nothing but the resty upgrade. Was fine for two years. I cannot prove it as I could not recreate the bug but a downgrade to 2.9.1 immediately solved the problem.

It could be a side effect of this buffer problem but as i cannot prove it i won't open a new thread. Just wanted to state it if somebody else experiences something different and has more time at hands to test it through.

Here is an example log from our kibana:

2023/11/28 13:51:16.736214 WARN RESTY parse "{"some very large json request body here"}/oms/customer/v3/users/2133478": first path segment in URL cannot contain colon, Attempt 1

Because of the WARN RESTY you can see the warning is logged from the resty code base as this log is hard coded there.

Here is my example gist where i could not recreate the problem and my time budget was done: https://gist.github.com/niksteff/2fd29672a24372782627d99ba2016031

Coronon commented 9 months ago

For everyone that needs a quick fix without downgrading to v2.9.1:

replace github.com/go-resty/resty/v2 => github.com/lattwood/resty/v2 v2.0.0-20231102200459-74e9e135ae0c

Put the above line into your go.mod file to override the resty code with this PR.

vithubati commented 9 months ago

Any timeline as to when this PR will be merged?

jeevatkm commented 9 months ago

@lattwood and members on the PR - Thanks for reaching out. I have been traveling on vacation days and will return from vacation in the first week of January. I'm sorry for not checking my emails and notifications properly these days.

Let me merge this PR and make a release.

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (105f718) 96.51% compared to head (74e9e13) 96.51%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## v2 #745 +/- ## ========================================== - Coverage 96.51% 96.51% -0.01% ========================================== Files 12 12 Lines 1637 1636 -1 ========================================== - Hits 1580 1579 -1 Misses 36 36 Partials 21 21 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.