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.

Sign up for GitHub

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

Jump to bottom

directory recv: add sanity checks before writing into the destination #86

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

directory recv: add sanity checks before writing into the destination #86

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06dcf92
directory recv: add sanity checks before writing into the destination
vu3rdd Jul 13, 2022
09fb564
be more tolerant of clients that may have off-by-one errors in offer …
vu3rdd Aug 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions cmd/recv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,21 @@ func recvAction(cmd *cobra.Command, args []string) {
bail("Read zip error: %s", err)
}

// calculate the uncompressed size of the contents of the zip
var actualUncompressedSize uint64
var fileCount int
for _, f := range zr.File {
actualUncompressedSize += f.FileHeader.UncompressedSize64
Copy link
Owner

Choose a reason for hiding this comment

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

For a malicious zipfile I don't think we can trust the value of UncompressedSize64

fileCount++
}
if msg.UncompressedBytes64 < int64(actualUncompressedSize) {
bail("error: uncompressed size of the directory mismatch with that in the offer message")
}

if msg.FileCount < fileCount {
bail("error: number of files in the directory mismatch with that in the offer message")
}

Comment on lines +202 to +216
Copy link
Contributor

@Jacalz Jacalz Dec 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I realised that running fileCount++ in the loop actually is quite unnecessary. We can just use the size of the slice instead and avoid one add instruction for each loop iteration.

Suggested change
// calculate the uncompressed size of the contents of the zip
var actualUncompressedSize uint64
var fileCount int
for _, f := range zr.File {
actualUncompressedSize += f.FileHeader.UncompressedSize64
fileCount++
}
if msg.UncompressedBytes64 < int64(actualUncompressedSize) {
bail("error: uncompressed size of the directory mismatch with that in the offer message")
}
if msg.FileCount < fileCount {
bail("error: number of files in the directory mismatch with that in the offer message")
}
if msg.FileCount < len(zr.File) {
bail("error: number of files in the directory mismatch with that in the offer message")
}
// calculate the uncompressed size of the contents of the zip
var actualUncompressedSize uint64
for _, f := range zr.File {
actualUncompressedSize += f.FileHeader.UncompressedSize64
}
if msg.UncompressedBytes64 < int64(actualUncompressedSize) {
bail("error: uncompressed size of the directory mismatch with that in the offer message")
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the suggestion as I realised it actually makes more sense to check file counts before checking uncompressed size in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vu3rdd Any chance to get this improved so the PR perhaps can be merged after that?

for _, zf := range zr.File {
p, err := filepath.Abs(filepath.Join(dirName, zf.Name))
if err != nil {
Expand Down