Potential false positive results

[Hipapheralkus]

I was shown a Certain confidence finding with "Malicious Excel upload/download". However, the Response 2 is basically showing only pdf:

HTTP/1.1 200 OK
connection: close
content-length: 26112
content-type: application/pdf;charset=UTF-8
x-content-type-options: nosniff
content-disposition: inline; filename="test.pdf"

Even though I understand this (and other) findings which were shown, I can't picture a usecase (apart from forcing user to change the file extension to something else after downloading) where this could be exploited. True, it should not be possible and the server should perform some content-based validations for PDF files, but still. Apart from Excel, other types were malicious CSV, IQY, and few others which requires opening file with some special software (not pdf reader). I don't know about the definition of False Positive here, and maybe you are aware, maybe it is a bug, so I rather point it out loud for you to have a look.

[floyd-fuh]

Thanks, good feedback.

Yeah, the confidence rating was a little bit off for the "Malicous download" type of issues. I've fixed it in my local code, that will change in a future version to Tentative.

On the other hand you hit one of the false positive cases, as I simply don't check content-disposition contents or content-type. The .pdf file is only uploaded because your originally uploaded file had a pdf extension. On the other hand while this exact response indicates a false positive, I would like the user to notice that this might be an issue if you are able to change the .pdf to .pdf;.xls or such (IE).

I would have to check content-type and content-disposition, which is currently not supported in the way we need it here. This might be a feature in the future, but for now it's just going to be "Tentative".

[Hipapheralkus]

Just a quick update, I was able to easily change the

Content-Disposition: form-data; name="fileName"

legit.csv

which went through and then the response headers were of proper csv. Perhaps an extended feature request could be for the extension to try to upload file using different extensions/content types in the request and observe which of them go through (I had response codes 201 vs 500 for valid/invalid extension) for the engine to find out which files are supported for upload?

[floyd-fuh]

So it wasn't a false positive after all, that's why I think this is not a big issue at the moment.

So the malicious CSV upload already does upload .csv extension, but as well with the original extension. As the detection mechanism doesn't check for the content-type or content-disposition, it will pick up either of them as long it is the correct CSV content, in your case it picked up the one that less clearly indicate a security issue. One idea is that I should probably change the order of how they are sent, as sending the original extension is likely to pass validations but also more likely to create a false positive (and is currently done first). So this should be done at the end, because then it can pick up the .csv first.

The other thing you suggest of analyzing which extension/content types go through is already a TODO in the code for a new "analyzer module":

            # TODO feature: "Analyzer module"
            # new module that uploads a png, a jpeg, a gif, etc. and checks in the downloaded
            # content which byte sequences of a certain length (eg. 6) survived transformation on the server
            # basically we could use something like python's SequenceMatcher to check where the files match...
            # Additionally, make the module analyze certain things such as "if we upload a PNG, is the
            # returned content-type in the redownloader a PNG?" with other types as well
            # What would also be a nice feature is to upload a PNG and download it again. Then use that PNG
            # as a starting point for attacks as we can be sure that is a valid one.

However, in the end we have the problem that most websites will not clearly indicate if the upload was successful with an HTTP status of 500, therefore we would probably need to check the response for an error code or simliar which would then need more new UI fields etc. etc. which will lead to a huge amount of code and more confusion for most users of the extension. That's why I would like to implement something like this as a module (which will generate an informational issue telling you what the extension thinks can be uploaded), but I will probably never implement it for all other modules.

On a side note, I don't think this extension will ever be a one stop solution, so what you did there with changing a file extension to something else is always going to be necessary to confirm a security issue.

[floyd-fuh]

The confidence rating will change when https://github.com/PortSwigger/upload-scanner/pull/6 is merged