-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Using upload buttons for web server instead of file referencing #1552
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.
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
7071d44
04ab9fe
b4707d8
bca69a3
8562727
47dbe59
e363922
a1a6c62
bbd8bf0
24ece8c
2f6d2c8
5c637d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,17 @@ | |
| import multiprocessing | ||
| import os | ||
| import random | ||
| import re | ||
| import secrets | ||
| import string | ||
| import time | ||
| import uuid | ||
| from threading import Thread | ||
| from types import SimpleNamespace | ||
|
|
||
| from flask import Flask, Response, abort, jsonify, make_response, render_template | ||
| from flask import request as flask_request | ||
| from itsdangerous import BadData, URLSafeTimedSerializer | ||
| from werkzeug.serving import WSGIRequestHandler | ||
| from werkzeug.utils import secure_filename | ||
|
|
||
|
|
@@ -50,6 +54,20 @@ | |
|
|
||
| app = Flask(__name__, template_folder=str(Config.path.web_static_dir)) | ||
| app.config.from_object(__name__) | ||
| app.config["MAX_CONTENT_LENGTH"] = ( | ||
| 10 * 1024 * 1024 | ||
| ) # https://flask.palletsprojects.com/en/stable/patterns/fileuploads/ | ||
| FILE_UPLOAD_PARAMS = ("targets_list", "passwords_list", "usernames_list", "read_from_file") | ||
| # Uploaded files are referenced by a signed, self-expiring token instead of a raw | ||
| # filename, so /new/scan only trusts paths that /upload/file actually issued (for the | ||
| # matching parameter) and never has to keep any server-side upload state. | ||
| UPLOAD_TOKEN_TTL_SECONDS = 15 * 60 | ||
| _upload_token_serializer = URLSafeTimedSerializer( | ||
| secrets.token_hex(32), salt="nettacker-file-upload" | ||
| ) | ||
| # Only files written by upload_file() match this prefix (uuid4 hex + "_"); the cleanup | ||
| # sweep is scoped to it so it can never touch reports or other tmp artifacts. | ||
| UPLOAD_FILENAME_RE = re.compile(r"^[0-9a-f]{32}_") | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| nettacker_path_config = Config.path | ||
| nettacker_application_config = Config.settings.as_dict() | ||
|
|
@@ -236,6 +254,62 @@ def sanitize_report_path_filename(report_path_filename): | |
| return safe_report_path | ||
|
|
||
|
|
||
| def allowed_file(filename): | ||
| return ( | ||
| "." in filename | ||
| and filename.rsplit(".", 1)[1].lower() in Config.settings.allowed_upload_extensions | ||
| ) | ||
|
|
||
|
|
||
| def cleanup_expired_uploads(): | ||
| """Delete upload temp files whose token has expired. | ||
|
|
||
| Files that were uploaded but never submitted to a scan are otherwise never | ||
| cleaned up. The sweep is opportunistic (run on every upload) so it needs no | ||
| background task or shared state, and is scoped to the upload naming prefix so | ||
| it can never remove reports or other tmp artifacts. | ||
| """ | ||
| tmp_dir = nettacker_path_config.tmp_dir | ||
| if not tmp_dir.is_dir(): | ||
| return | ||
| cutoff = time.time() - UPLOAD_TOKEN_TTL_SECONDS | ||
| for entry in tmp_dir.iterdir(): | ||
| try: | ||
| if ( | ||
| entry.is_file() | ||
| and UPLOAD_FILENAME_RE.match(entry.name) | ||
| and entry.stat().st_mtime < cutoff | ||
| ): | ||
| entry.unlink(missing_ok=True) | ||
| except OSError: | ||
| continue | ||
|
|
||
|
|
||
| @app.route("/upload/file", methods=["POST"]) | ||
| def upload_file(): | ||
| api_key_is_valid(app, flask_request) | ||
| cleanup_expired_uploads() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because cleanup only runs at the start of Useful? React with 👍 / 👎. |
||
| param_name = flask_request.form.get("param_name", "") | ||
| if param_name not in FILE_UPLOAD_PARAMS: | ||
| return jsonify(structure(status="error", msg=_("upload_invalid_param"))), 400 | ||
| if "file" not in flask_request.files: | ||
| return jsonify(structure(status="error", msg=_("upload_no_file"))), 400 | ||
| uploaded = flask_request.files["file"] | ||
| if uploaded.filename == "": | ||
| return jsonify(structure(status="error", msg=_("upload_no_file_selected"))), 400 | ||
| if not allowed_file(uploaded.filename): | ||
| return jsonify(structure(status="error", msg=_("upload_file_type_not_allowed"))), 400 | ||
| filename = secure_filename(uploaded.filename) | ||
| if not filename: | ||
| return jsonify(structure(status="error", msg=_("upload_invalid_filename"))), 400 | ||
| stored_name = f"{uuid.uuid4().hex}_{filename}" | ||
| tmp_dir = nettacker_path_config.tmp_dir | ||
| tmp_dir.mkdir(parents=True, exist_ok=True) | ||
| uploaded.save(str(tmp_dir / stored_name)) | ||
| token = _upload_token_serializer.dumps({"param": param_name, "name": stored_name}) | ||
| return jsonify(structure(status="ok", msg=token)), 200 | ||
|
|
||
|
|
||
| @app.route("/new/scan", methods=["GET", "POST"]) | ||
| def new_scan(): | ||
| """ | ||
|
|
@@ -253,6 +327,28 @@ def new_scan(): | |
| if not report_path_filename: | ||
| return jsonify(structure(status="error", msg="Invalid report filename")), 400 | ||
| form_values["report_path_filename"] = str(report_path_filename) | ||
|
|
||
| uploaded_paths = [] | ||
| for key in FILE_UPLOAD_PARAMS: | ||
| token = form_values.get(key) | ||
| if not token: | ||
| form_values.pop(key, None) | ||
| continue | ||
| try: | ||
| payload = _upload_token_serializer.loads(token, max_age=UPLOAD_TOKEN_TTL_SECONDS) | ||
| except BadData: | ||
| # tampered, forged, or expired token | ||
| return jsonify(structure(status="error", msg=_("upload_token_invalid"))), 400 | ||
| if not isinstance(payload, dict) or payload.get("param") != key: | ||
| # token was issued for a different parameter | ||
| return jsonify(structure(status="error", msg=_("upload_token_invalid"))), 400 | ||
| stored_name = secure_filename(payload.get("name", "")) | ||
| file_path = nettacker_path_config.tmp_dir / stored_name | ||
| if not stored_name or not file_path.is_file(): | ||
| return jsonify(structure(status="error", msg=_("upload_file_not_found"))), 400 | ||
| form_values[key] = str(file_path) | ||
| uploaded_paths.append(file_path) | ||
|
|
||
| for key in nettacker_application_config: | ||
| if key not in form_values: | ||
| form_values[key] = nettacker_application_config[key] | ||
|
|
@@ -263,7 +359,11 @@ def new_scan(): | |
| ] | ||
| # Handle service discovery | ||
| form_values["skip_service_discovery"] = form_values.get("skip_service_discovery", "") == "true" | ||
| nettacker_app = Nettacker(api_arguments=SimpleNamespace(**form_values)) | ||
| try: | ||
| nettacker_app = Nettacker(api_arguments=SimpleNamespace(**form_values)) | ||
| finally: | ||
| for file_path in uploaded_paths: | ||
| file_path.unlink(missing_ok=True) | ||
|
Comment on lines
+351
to
+352
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the uploaded token is for Useful? React with 👍 / 👎. |
||
| app.config["OWASP_NETTACKER_CONFIG"]["options"] = nettacker_app.arguments | ||
| thread = Thread(target=nettacker_app.run) | ||
| thread.start() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -320,8 +320,52 @@ $(document).ready(function () { | |
| } | ||
| }); | ||
|
|
||
| // file upload handler | ||
| var uploaded_tokens = {}; | ||
| var pendingUploads = 0; | ||
| $.each(["targets_list", "usernames_list", "passwords_list", "read_from_file"], function(_, param) { | ||
| $("#" + param + "_file").change(function () { | ||
| var fileInput = this; | ||
| if (!fileInput.files.length) return; | ||
| var formData = new FormData(); | ||
| formData.append("file", fileInput.files[0]); | ||
| formData.append("param_name", param); | ||
| var statusSpan = $("#" + param + "_status"); | ||
| statusSpan.text("Uploading..."); | ||
| pendingUploads++; | ||
| $.ajax({ | ||
| type: "POST", | ||
| url: "/upload/file", | ||
| data: formData, | ||
| processData: false, | ||
| contentType: false, | ||
| }) | ||
| .done(function (res) { | ||
| uploaded_tokens[param] = res.msg; | ||
| statusSpan.text("Uploaded: " + fileInput.files[0].name); | ||
| }) | ||
| .fail(function (jqXHR) { | ||
| delete uploaded_tokens[param]; | ||
| statusSpan.text("Upload failed"); | ||
| fileInput.value = ""; | ||
| }) | ||
| .always(function () { | ||
| pendingUploads--; | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| // submit new scan | ||
| $("#submit_new_scan").click(function () { | ||
| // block submission while file uploads are still in flight, otherwise the | ||
| // scan is submitted without the uploaded file tokens and silently misses them | ||
| if (pendingUploads > 0) { | ||
| document.getElementById("error_msg").innerHTML = | ||
| "Please wait for file uploads to finish before submitting."; | ||
| $("#failed_request").removeClass("hidden"); | ||
| setTimeout('$("#failed_request").addClass("hidden");', 5000); | ||
| return; | ||
| } | ||
| // set variables | ||
| // check ranges | ||
| if (document.getElementById("scan_ip_range").checked) { | ||
|
|
@@ -417,12 +461,21 @@ $(document).ready(function () { | |
| } | ||
| } | ||
|
|
||
| $.extend(data, uploaded_tokens); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For the new usernames upload, this adds Useful? React with 👍 / 👎. |
||
|
|
||
| $.ajax({ | ||
| type: "POST", | ||
| url: "/new/scan", | ||
| data: data, | ||
| }) | ||
| .done(function (res) { | ||
| // the server deletes the uploaded temp files once the scan starts, so | ||
| // clear the consumed tokens to avoid re-sending stale ones on the next scan | ||
| uploaded_tokens = {}; | ||
| $.each(["targets_list", "usernames_list", "passwords_list", "read_from_file"], function (_, param) { | ||
| $("#" + param + "_status").text(""); | ||
| $("#" + param + "_file").val(""); | ||
| }); | ||
| var results = JSON.stringify(res); | ||
| results = results.replaceAll(",", ",<br>"); | ||
| document.getElementById("success_msg").innerHTML = results; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reviewed commit
9d1383ddce47c4bc77a88ed59b1cd3e24fd59746is unsigned (git log -1 --format='%G?'reportsN), but this repository requires all commits to be signed; please rewrite or squash the PR commit with signing enabled before merge.Useful? React with 👍 / 👎.