Open
Bug 834540
Opened 11 years ago
Updated 2 years ago
Do more validation of zip file names
Categories
(Core :: Networking: JAR, defect, P5)
Core
Networking: JAR
Tracking
()
NEW
People
(Reporter: jduell.mcbugs, Unassigned)
References
Details
(Keywords: sec-audit, Whiteboard: [necko-would-take])
Attachments
(1 file)
4.51 KB,
patch
|
taras.mozilla
:
review-
|
Details | Diff | Splinter 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?
Comment 3•11 years ago
|
||
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-
Reporter | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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-
Updated•8 years ago
|
Whiteboard: [necko-would-take]
Comment 6•7 years ago
|
||
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•