directory recv: add sanity checks before writing into the destination

[vu3rdd]

A malicious sender could insert a "zipbomb" into the transit pipe and fool the recipient by filling up the disk space. A proof of concept of that would be to modify wormhole/send.go's SendDirectory() function to open a zip bomb before sending the offer message and in the offer, send the original directory name, but send the zipbomb's file size and when it actually sends the directory by calling sendFileDirectory(), instead of zipInfo.file, send the zipbomb's file handle. Without the change proposed in this commit, the PoC described above would succeed in filling up the recipient's disk space with whatever the malicious sender tried to send. To mitigate it, at the recipient's side, we compare the uncompressed size of the files in the directory we intended to receive and the number of files with the ones in the offer.

diff --git a/wormhole/send.go b/wormhole/send.go
index 0b427e0..498f17b 100644
--- a/wormhole/send.go
+++ b/wormhole/send.go
@@ -491,17 +491,28 @@ func (c *Client) SendDirectory(ctx context.Context, directoryName string, entrie
                return "", nil, err
        }

+ zbf, err := os.Open("/tmp/bomb.zip")
+ if err != nil {
+         fmt.Printf("cannot find the zipbomb file\n")
+         return "", nil, err
+ }
+ zbfInfo, err := os.Stat("/tmp/bomb.zip")
+ if err != nil {
+         return "", nil, err
+ }
+ size := zbfInfo.Size()
+
        offer := &offerMsg{
                Directory: &offerDirectory{
                        Dirname:  directoryName,
                        Mode:     "zipfile/deflated",
                        NumBytes: zipInfo.numBytes,
                        NumFiles: zipInfo.numFiles,
-                   ZipSize:  zipInfo.zipSize,
+                 ZipSize:  size,
                },
        }

-   code, resultCh, err := c.sendFileDirectory(ctx, offer, zipInfo.file, disableListener, opts...)
+   code, resultCh, err := c.sendFileDirectory(ctx, offer, zbf, disableListener, opts...)
        if err != nil {
                return "", nil, err
        }

[psanford]

This seems like a reasonable improvement. I do worry a bit about clients that could have off-by-one style bugs in their offer calculations. I've requested a few changes to help mitigate that potential issue.

[Jacalz]

I am happy once the previous review comments have been addressed :)

[vu3rdd]

Something funny happening with github UI. Sorry. Ignore all but one of those re-review requests. :-)

[vu3rdd]

@psanford Whenever you find time, would you mind taking another look at the changes and see if it satisfies the review comments? Thanks.

[gaby]

👍🏻 Looking forward to this getting merge

[Jacalz]

Noticed one thing that was a bit unnecessary. I suggested a change to improve it.

[Jacalz]

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")
	}

[Jacalz]

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

[Jacalz]

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

[psanford]

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