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.

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

registry/{storage,handlers}: limit content sizes #2340

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

stevvooe
Copy link
Collaborator

Under certain circumstances, the use of StorageDriver.GetContent can
result in unbounded memory allocations. In particualr, this happens when
accessing a layer through the manifests endpoint.

This problem is mitigated by setting a 4MB limit when using to access
content that may have been accepted from a user. In practice, this means
setting the limit with the use of BlobProvider.Get by wrapping
StorageDriver.GetContent in a helper that uses StorageDriver.Reader
with a limitReader that returns an error.

When mitigating this security issue, we also noticed that the size of
manifests uploaded to the registry is also unlimited. We apply similar
logic to the request body of payloads that are full buffered.

Signed-off-by: Stephen J Day stephen.day@docker.com

cc @riyazdf @dmcgowan

Under certain circumstances, the use of `StorageDriver.GetContent` can
result in unbounded memory allocations. In particualr, this happens when
accessing a layer through the manifests endpoint.

This problem is mitigated by setting a 4MB limit when using to access
content that may have been accepted from a user. In practice, this means
setting the limit with the use of `BlobProvider.Get` by wrapping
`StorageDriver.GetContent` in a helper that uses `StorageDriver.Reader`
with a `limitReader` that returns an error.

When mitigating this security issue, we also noticed that the size of
manifests uploaded to the registry is also unlimited. We apply similar
logic to the request body of payloads that are full buffered.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
Copy link
Contributor

@riyazdf riyazdf left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronlehmann
Copy link
Contributor

LGTM

@codecov
Copy link

codecov bot commented Jul 20, 2017

Codecov Report

Merging #2340 into master will decrease coverage by 9.62%.
The diff coverage is 51.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2340      +/-   ##
==========================================
- Coverage   60.25%   50.62%   -9.63%     
==========================================
  Files         125      126       +1     
  Lines       14353    14390      +37     
==========================================
- Hits         8648     7285    -1363     
- Misses       4821     6357    +1536     
+ Partials      884      748     -136
Impacted Files Coverage Δ
registry/handlers/manifests.go 55.27% <0%> (-0.16%) ⬇️
registry/handlers/blobupload.go 45.2% <0%> (ø) ⬆️
registry/storage/blobstore.go 57.89% <100%> (ø) ⬆️
registry/handlers/helpers.go 39.53% <100%> (+7.03%) ⬆️
registry/storage/io.go 48.48% <48.48%> (ø)
registry/storage/driver/gcs/gcs.go 0.32% <0%> (-66.13%) ⬇️
registry/storage/driver/oss/oss.go 0.45% <0%> (-56.5%) ⬇️
registry/storage/driver/s3-aws/s3.go 4.07% <0%> (-55.4%) ⬇️
registry/storage/driver/s3-goamz/s3.go 0.4% <0%> (-52.4%) ⬇️
registry/client/transport/transport.go 68.75% <0%> (-8.75%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cfdfbd...55ea440. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants