Show error on copied file above context directory in build#2149
Merged
rhatdan merged 1 commit intocontainers:masterfrom Feb 11, 2020
Merged
Show error on copied file above context directory in build#2149rhatdan merged 1 commit intocontainers:masterfrom
rhatdan merged 1 commit intocontainers:masterfrom
Conversation
Member
|
Didn't try locally but |
Member
|
A couple of small nits, but CI system seems to be hosed up. |
rhatdan
reviewed
Feb 10, 2020
imagebuildah/stage_executor.go
Outdated
| // directory. Also raise an error if the src escapes | ||
| // the context directory. | ||
| contextSrc, err := securejoin.SecureJoin(contextDir, src) | ||
| if strings.HasPrefix(src, "../") && err == nil { |
Member
There was a problem hiding this comment.
nit: Should reverse order and check if err == nil first, for speed, although maybe compiler is smart enough.
Member
Author
There was a problem hiding this comment.
touched up here and line 445
Member
Author
There was a problem hiding this comment.
just reversed here and at line 445
| @@ -0,0 +1,3 @@ | |||
| This is a text file to be used in Buildah testing. | |||
| This will be used to ensure that a file from above the context | |||
| directory can not be copied during the buildh phase. | |||
Member
Author
There was a problem hiding this comment.
apparently a rogue space at the end of this line caused tests to be unhappy. Also touched up the "buildh" typo
When a COPY command in a Container file looked like "../file.txt"
SecureJoin would secure the file by lopping off the "../".
However, the code would then append that file name to the passed in
context directory and look for the file. That would fail as in most
cases there was no `{context-dir}/file.txt`, rather the file was at
`{context-dir}/../file.txt`. Using a relative directory like this
outside of the context directory can be a security risk. Docker
doesn't allow it nor should we.
This change now errors out when a file that starts with `../` is
presented as a copy from target.
Addresses: containers/podman#5035
Signed-off-by: TomSweeneyRedHat <tsweeney@redhat.com>
dc9b5fc to
13dcf6d
Compare
Member
Author
|
Happy green test buttons |
Member
|
bors r+ |
Contributor
|
👎 Rejected by too few approved reviews |
rhatdan
approved these changes
Feb 11, 2020
Member
|
bors retry |
bors bot
added a commit
that referenced
this pull request
Feb 11, 2020
2149: Show error on copied file above context directory in build r=rhatdan a=TomSweeneyRedHat
When a COPY command in a Container file looked like "../file.txt"
SecureJoin would secure the file by lopping off the "../".
However, the code would then append that file name to the passed in
context directory and look for the file. That would fail as in most
cases there was no `{context-dir}/file.txt`, rather the file was at
`{context-dir}/../file.txt`. Using a relative directory like this
outside of the context directory can be a security risk. Docker
doesn't allow it nor should we.
This change now errors out when a file that starts with `../` is
presented as a copy from target.
Addresses: containers/podman#5035
Signed-off-by: TomSweeneyRedHat <tsweeney@redhat.com>
Co-authored-by: TomSweeneyRedHat <tsweeney@redhat.com>
Contributor
Build failed
|
Member
|
Bors seems to be acting out, for no reason. Manually merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a COPY command in a Container file looked like "../file.txt"
SecureJoin would secure the file by lopping off the "../".
However, the code would then append that file name to the passed in
context directory and look for the file. That would fail as in most
cases there was no
{context-dir}/file.txt, rather the file was at{context-dir}/../file.txt. Using a relative directory like thisoutside of the context directory can be a security risk. Docker
doesn't allow it nor should we.
This change now errors out when a file that starts with
../ispresented as a copy from target.
Addresses: containers/podman#5035
Signed-off-by: TomSweeneyRedHat tsweeney@redhat.com