Hello @rdmark!

I work at Sonar as an AppSec researcher and am part of the team responsible for reviewing SonarCloud feedback.
You shared feedback on an issue affecting this PR's code a few days ago that this issue is a false positive.

If you do not mind, could you tell me what evidence would change your mind? Or what evidence (or explanation) would have changed your mind when you first saw this problem?

Thank you,

Loris

@loris-s-sonarsource Hi, thanks for following up! So the code in question in this:

    destination = request.form.get("destination")
    if destination == "disk_images":
        server_info = piscsi_cmd.get_server_info()
        destination_dir = server_info["image_dir"]
    elif destination == "shared_files":
        destination_dir = FILE_SERVER_DIR
    elif destination == "piscsi_config":
        destination_dir = CFG_DIR
    else:
        return make_response(f"Invalid destination '{destination}'", 403)

The Sonar scan suggested that the client can feed malicious data into this code for unexpected behaviors. However, my analysis was that we have these four branches here, the first three checking against a string literal and assigning a fixed value based on which one is detected. And the fourth branch falls back to an error when no known sting literal is found. We don't use the data coming from the client directly as an input to anything else.

Each of those first three options are available to the user in the client, so we're not trying to obfuscate either option. So I just couldn't see how any malicious data could lead to anything else but 1) one of the three valid positive outcomes, or the error when the malicious data doesn't match either of the three string literals.

Is there an attack vector here that I'm missing?

@rdmark You're correct that the destination dir cannot be manipulated by tampering with the input. I think it's being flagged because make_response isn't sanitising the input value before returning it as part of the error message, e.g.

The response is escaped by the Dropzone library in the web UI:

However, if we were able to submit the form (for whatever reason), then the browser will render any HTML (malicious script tag, etc) when it receives the response from POST /files/upload:

I see the point, very helpful demonstration. The handling of user controlled data in UI strings and logging is definitely a security blind spot of mine. The easy solution here is to not display the destination string in the UI message. It is only marginally useful anyways.

Hi! Thanks for taking over as I'm French and was in a demonstration/on strike yesterday for our pension.

So yeah, exactly @nucleogenic. Something to take into account is that our SAST tool does not take into account the business impact, so it is possible that this XSS actually bears no impact in the web app (but it's not ours to say 😄). We do not have a way to differentiate static web apps that show pictures of cats from hyper-critical healthcare systems for now.

If you still want to show the destination string in the UI message, or for future equivalent issues where you absolutely need to add user-controlled text, you can also play around with mime-types. Setting as text/plain for example will make the user-controlled data non-exploitable:

@app.route("/")
def hello_world():
    destination = request.args.get('dest', '')
    resp = make_response(f"Invalid destination '{destination}'", 403)
    resp.mimetype = "text/plain"

    return resp

But there are some pitfalls, here is a copy-paste from the pitfalls we enumerate in the rule (you can see it in the SC issue > How to fix it > Flask tab > scroll down a little) :

Pitfalls

Content-types

Be aware that there are more content-types than text/html that allow to execute JavaScript code in a browser and thus are prone to cross-site scripting vulnerabilities.
The following content-types are known to be affected:

• application/mathml+xml
• application/rdf+xml
• application/vnd.wap.xhtml+xml
• application/xhtml+xml
• application/xml
• image/svg+xml
• multipart/x-mixed-replace
• text/html
• text/rdf
• text/xml
• text/xsl

Cheers!

Loris

@rdmark if this is a blind spot of yours, I recommend reading the contents of the issue tabs (here it is). We tried to add as much info as concisely possible in the The Why is this an issue and How to fix it sections. (I hope it's concise enough haha).

Thank you both for your openness to this discussion btw !

Cheers,

Loris