Open Bug 834540 Opened 11 years ago Updated 2 years ago

Do more validation of zip file names

Categories

(Core :: Networking: JAR, defect, P5)

defect

Tracking

()

People

(Reporter: jduell.mcbugs, Unassigned)

References

Details

(Keywords: sec-audit, Whiteboard: [necko-would-take])

Attachments

(1 file)

Attached patch v1Splinter Review
Fuller validation of file names in zip archives, per bug 832162 comment 14.

Not sure this needs to be hidden?  No known exploits at this time.
Attachment #706182 - Flags: review?
Doesn't need to be hidden
Group: core-security
Keywords: sec-audit
Seems like something that should have tests?
Flags: in-testsuite?
Comment on attachment 706182 [details] [diff] [review]
v1

Review of attachment 706182 [details] [diff] [review]:
-----------------------------------------------------------------

Your review request didn't have any requestee set. I looked at the patch and I have a question:

::: modules/libjar/nsZipArchive.cpp
@@ +571,5 @@
> +    if (name[0] == '/' ||
> +        PL_strnchr(name, '\\', namelen) ||
> +        PL_strnlen(name, namelen) != namelen) {
> +      return NS_ERROR_FILE_CORRUPTED;
> +    }

I see below you removed the code that you added in an earlier patch that checks for empty path components. I think you were intending to add it here, but I don't see how empty path components are protected against here. Am I overlooking something? (qrefresh?)
Attachment #706182 - Flags: review? → review-
Comment on attachment 706182 [details] [diff] [review]
v1

Review of attachment 706182 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libjar/nsZipArchive.cpp
@@ +571,5 @@
> +    if (name[0] == '/' ||
> +        PL_strnchr(name, '\\', namelen) ||
> +        PL_strnlen(name, namelen) != namelen) {
> +      return NS_ERROR_FILE_CORRUPTED;
> +    }

Since the code in this patch barfs if we see an entry that starts with "/", that means we won't see empty path components beyond this point, so I removed the check in the later codepath.
Attachment #706182 - Flags: review- → review?(taras.mozilla)
Comment on attachment 706182 [details] [diff] [review]
v1

-        // The character before this is '/', so if this is also '/' then we
-        // have an empty path component. Skip it.
-        if (name[dirlen] == '/')
-          continue;
-

I see this removed, but I don't see an equivalent check for empty components of form dir//fname
Attachment #706182 - Flags: review?(taras.mozilla) → review-
Whiteboard: [necko-would-take]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: