Additional validation on malformed images #204
Conversation
|
|
||
| curImgID := imgID | ||
|
|
||
| var err error | ||
| for curImgID != "" { | ||
| if _, ok := deps[curImgID]; ok { |
There was a problem hiding this comment.
nit, since you're using a bool you can just use the default value check (if deps[curImgID])
lucab
force-pushed
the
to-upstream/cve-fixes
branch
from
feff9fe
to
5a82054
Compare
|
@jonboulle rebased to address the nit and to include the reference to the second CVE. /cc @thebeeman |
| @@ -60,3 +63,21 @@ func ParseDockerURL(arg string) (*ParsedDockerURL, error) { | |||
| p, err := docker.ParseDockerURL(arg) | |||
| return (*ParsedDockerURL)(p), err | |||
| } | |||
|
|
|||
| // IsValidLayerRefs validates a layer refs | |||
There was a problem hiding this comment.
This was supposed to be a library-exposed helper, but I guess we may re-introduce it later as soon as any consumers need it.
| @@ -279,14 +282,24 @@ func extractEmbeddedLayer(file *os.File, layerID string, outputPath string) (*os | |||
| return layerFile, nil | |||
| } | |||
|
|
|||
| // getAncestry computes an image ancestry, returning an ordered list | |||
| // of dependencies starting from the topmost image to the base. | |||
| // It checks for dependency loops via duplicates detection in the image | |||
| ancestry = append(ancestry, curImgID) | ||
| log.Debug(`Getting ancestry for layer "`, curImgID, `".`) |
There was a problem hiding this comment.
IMO nicer to just format the string here: fmt.Sprintf("... %q ...", curImgID)
| return nil | ||
| } | ||
|
|
||
| // IsValidLayerId validates a layer ID |
There was a problem hiding this comment.
Naming nit: generally IsFoo functions return a bool. Instead perhaps AssertValidLayerId or ValidateLayerID
There was a problem hiding this comment.
My bad, this was just returning a bool at first but then changed to error returning, but I forgot to reflect that in the naming.
lucab
force-pushed
the
to-upstream/cve-fixes
branch
from
5a82054
to
60c1c09
Compare
This commit fixes a possible infinite loop while traversing the dependency ancestry of a malformed local image file. This has been assigned CVE-2016-8579: appc#203 (comment)
lucab
force-pushed
the
to-upstream/cve-fixes
branch
from
60c1c09
to
698ecaa
Compare
|
Further iteration on this: removed unused bits, renamed function and cleaned the debug printing. |
|
|
||
| // ValidateLayerId validates a layer ID | ||
| func ValidateLayerId(id string) error { | ||
| validId := regexp.MustCompile(`^(\w+:)?([A-Fa-f0-9]+)$`) |
There was a problem hiding this comment.
Sorry, missed this on previous pass: this should a global variable (so failure would be at initialization time, not function invocation time)
There was a problem hiding this comment.
Ack, moved to a global var stanza.
lucab
force-pushed
the
to-upstream/cve-fixes
branch
from
698ecaa
to
e5b78da
Compare
This commit introduces validation of layer IDs while handling images, in order to avoid path traversal when unpacking layer content. This is typically mitigated for remote usecases by registry conformance, but can still be exploited locally. This has been assigned CVE-2016-7569: http://www.openwall.com/lists/oss-security/2016/09/28/8
lucab
force-pushed
the
to-upstream/cve-fixes
branch
from
e5b78da
to
500caa9
Compare
This PR introduces some additional validation before converting Docker images. This addresses:
It also introduces fixtures and tests script for such cases.
Fixes #201
Fixes #203