-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(reporting): harden Markdown filenames and remediate Go vulnerabilities #7633
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: dev
Are you sure you want to change the base?
Changes from 6 commits
865c90e
c03f5c8
efbb48c
06771d7
a20aa26
675fcc4
5371ac6
7f2254a
1ff643d
58d3ad6
b24ade5
b19e739
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 |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| # Markdown report filenames | ||
|
|
||
| The Markdown exporter writes one result file per finding. Result filenames are | ||
| constructed from the template ID, target host, a random UUID, and—when | ||
| available—the operator name: | ||
|
|
||
| ```text | ||
| <template-id>-<host>-<uuid>-<matcher-or-extractor>.md | ||
| ``` | ||
|
|
||
| The matcher name takes precedence when a result contains both a matcher name | ||
| and an extractor name. For extractor-only results, the extractor name is used. | ||
|
|
||
| ## Safety properties | ||
|
|
||
| Filename components can contain values derived from templates or scan targets, | ||
| so the exporter applies the following rules before writing a report: | ||
|
|
||
| 1. Path separators, parent-directory references, spaces, and characters that | ||
| are unsafe on common filesystems are replaced with underscores. | ||
| 2. Filenames are limited to 255 bytes. | ||
| 3. Truncation stops at a valid UTF-8 boundary. | ||
| 4. The UUID and `.md` extension are always retained when the template ID or | ||
| host must be truncated. | ||
| 5. The operator name is sanitized and truncated separately from the prefix. | ||
|
|
||
| Preserving the UUID prevents two findings with long, identical prefixes from | ||
| being truncated to the same filename and overwriting each other. Sanitizing | ||
| path separators and `..` prevents target-controlled values from escaping the | ||
| configured report directory. | ||
|
|
||
| ## Verification | ||
|
|
||
| The Markdown exporter tests cover: | ||
|
|
||
| - matcher-only and extractor-only filenames; | ||
| - matcher precedence when both operator names are present; | ||
| - UUID and operator suffix preservation for long names; | ||
| - UTF-8-safe truncation; and | ||
| - containment of filenames and sort-mode subdirectories within the report | ||
| directory. | ||
|
|
||
| Run the tests with: | ||
|
|
||
| ```console | ||
| go test ./pkg/reporting/exporters/markdown | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # Markdown and security change report | ||
|
|
||
| This file is a standalone, downloadable summary of the Markdown report filename | ||
| hardening and Go vulnerability remediation work. | ||
|
|
||
| ## Executive summary | ||
|
|
||
| The change set: | ||
|
|
||
| - prevents Markdown report paths from being influenced by directory traversal | ||
| sequences; | ||
| - prevents long template IDs and hosts from removing the per-finding UUID; | ||
| - includes the selected matcher or extractor name in result filenames; | ||
| - truncates filenames without producing invalid UTF-8; | ||
| - updates the Go toolchain and vulnerable dependencies; and | ||
| - reduces reachable `govulncheck` findings from 23 to one upstream advisory | ||
| with no fixed dependency version. | ||
|
|
||
| ## Markdown filename hardening | ||
|
|
||
| Markdown finding files use this structure: | ||
|
|
||
| ```text | ||
| <template-id>-<host>-<uuid>-<matcher-or-extractor>.md | ||
| ``` | ||
|
|
||
| The matcher name is selected when both a matcher and extractor name are | ||
| available. Extractor-only findings use the extractor name. | ||
|
|
||
| Before a file is written, unsafe filesystem characters, path separators, and | ||
| parent-directory references are replaced. The resulting filename is limited to | ||
| 255 bytes and truncated only at a valid UTF-8 boundary. Prefix truncation does | ||
| not remove the UUID or `.md` extension, so findings cannot collide merely | ||
| because their long template and host prefixes are identical. | ||
|
|
||
| ## Dependency remediation | ||
|
|
||
| | Component | Previous version | Updated version | | ||
| | --- | --- | --- | | ||
| | Go toolchain | 1.26.0 | 1.26.5 | | ||
| | `golang.org/x/text` | v0.38.0 | v0.39.0 | | ||
| | `github.com/yuin/goldmark` | v1.7.13 | v1.7.17 | | ||
| | Direct `github.com/google/go-github` use | v30.1.0 | v81.0.0 | | ||
|
|
||
| The direct GitHub client migration covers the custom-template downloader and | ||
| GitHub reporting tracker. Module metadata was refreshed after the upgrades. | ||
|
|
||
| ## Vulnerability scan result | ||
|
|
||
| The initial source scan reported 23 reachable vulnerabilities. After the | ||
| updates, the scan reports only GO-2026-5932. | ||
|
|
||
| GO-2026-5932 remains reachable through the latest available | ||
| `github.com/projectdiscovery/utils/update` dependency, which transitively uses | ||
| `github.com/google/go-github/v30` and `golang.org/x/crypto/openpgp`. The advisory | ||
| does not identify a fixed `golang.org/x/crypto` release. Eliminating this final | ||
| result requires the upstream update package to migrate away from the older | ||
| GitHub client or a replacement of that update subsystem. | ||
|
|
||
| Run the vulnerability scan with: | ||
|
|
||
| ```console | ||
| go run golang.org/x/vuln/cmd/govulncheck@latest \ | ||
| -db=https://storage.googleapis.com/go-vulndb ./... | ||
| ``` | ||
|
|
||
| An exit status indicating GO-2026-5932 is expected until the upstream | ||
| dependency is migrated. | ||
|
|
||
| ## Validation commands | ||
|
|
||
| ```console | ||
| go mod verify | ||
| go test ./pkg/reporting/exporters/markdown | ||
| go test ./pkg/external/customtemplates -run '^$' | ||
| go test ./pkg/reporting/trackers/github | ||
| go vet ./pkg/external/customtemplates \ | ||
| ./pkg/reporting/trackers/github \ | ||
| ./pkg/reporting/exporters/markdown | ||
| ``` | ||
|
|
||
| The `-run '^$'` custom-template command checks compilation without running the | ||
| network-dependent GitHub download tests. | ||
|
|
||
| ## Detailed documentation | ||
|
|
||
| - [Markdown report filenames](markdown-report-filenames.md) | ||
| - [Go vulnerability remediation](security-remediation.md) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| # Go vulnerability remediation | ||
|
|
||
| ## Scope | ||
|
|
||
| A `govulncheck` source scan was performed for all Go packages. The initial scan | ||
| reported 23 reachable vulnerabilities in the Go standard library and direct or | ||
| transitive dependencies. | ||
|
|
||
| The remediation updates the following components: | ||
|
|
||
| | Component | Previous | Remediated | Purpose | | ||
| | --- | --- | --- | --- | | ||
| | Go toolchain | 1.26.0 | 1.26.5 | Includes standard-library security fixes | | ||
| | `golang.org/x/text` | v0.38.0 | v0.39.0 | Fixes GO-2026-5970 | | ||
| | `github.com/yuin/goldmark` | v1.7.13 | v1.7.17 | Fixes GO-2026-5320 | | ||
| | `github.com/google/go-github` (direct use) | v30.1.0 | v81.0.0 | Removes Nuclei's direct dependency path through the deprecated OpenPGP API | | ||
|
|
||
| The GitHub client migration applies to the custom-template downloader and the | ||
| GitHub reporting tracker. | ||
|
|
||
| ## Verification command | ||
|
|
||
| The default Go vulnerability database endpoint may be unavailable in restricted | ||
| environments. The equivalent Google Cloud Storage database mirror can be used: | ||
|
|
||
| ```console | ||
| go run golang.org/x/vuln/cmd/govulncheck@latest \ | ||
| -db=https://storage.googleapis.com/go-vulndb ./... | ||
| ``` | ||
|
|
||
| After remediation, the scan reports one reachable advisory instead of 23. | ||
|
|
||
| ## Remaining upstream advisory | ||
|
|
||
| GO-2026-5932 remains reachable through | ||
| `github.com/projectdiscovery/utils/update`, which transitively depends on | ||
| `github.com/google/go-github/v30` and its use of | ||
| `golang.org/x/crypto/openpgp`. | ||
|
|
||
| The advisory describes the OpenPGP package as unmaintained and unsafe by | ||
| design, and the Go vulnerability database does not provide a fixed | ||
| `golang.org/x/crypto` version. Nuclei already uses `go-github/v81` for its own | ||
| GitHub API integrations, but the older module remains in the dependency graph | ||
| until `projectdiscovery/utils` migrates its update package. | ||
|
|
||
| The remaining advisory therefore cannot be resolved by upgrading | ||
| `golang.org/x/crypto`. It requires an upstream dependency migration or a local | ||
| replacement of the update subsystem. | ||
|
|
||
| ## Validation | ||
|
|
||
| Use the following checks when changing these dependencies: | ||
|
|
||
| ```console | ||
| go mod verify | ||
| go vet ./pkg/external/customtemplates \ | ||
| ./pkg/reporting/trackers/github \ | ||
| ./pkg/reporting/exporters/markdown | ||
| go test ./pkg/external/customtemplates -run '^$' | ||
| go test ./pkg/reporting/trackers/github \ | ||
| ./pkg/reporting/exporters/markdown | ||
| ``` | ||
|
|
||
| The custom-template package has tests that access external GitHub resources. | ||
| Using `-run '^$'` performs a compilation check without requiring network access. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "net/http" | ||
| "time" | ||
|
|
||
| "github.com/projectdiscovery/gologger" | ||
| "github.com/projectdiscovery/nuclei/v3/pkg/js/compiler" | ||
| "github.com/projectdiscovery/nuclei/v3/pkg/types" | ||
| "github.com/projectdiscovery/nuclei/v3/pkg/utils/json" | ||
|
|
@@ -22,26 +23,66 @@ type Concurrency struct { | |
| // Server represents the HTTP server that handles the concurrency settings endpoints. | ||
| type Server struct { | ||
| addr string | ||
| token string | ||
| config *types.Options | ||
| } | ||
|
|
||
| // New creates a new instance of Server. | ||
| func New(addr string, config *types.Options) *Server { | ||
| return &Server{ | ||
| addr: addr, | ||
| token: config.HttpApiToken, | ||
| config: config, | ||
| } | ||
| } | ||
|
|
||
| // Start initializes the server and its routes, then starts listening on the specified address. | ||
| // | ||
| // A dedicated ServeMux is used (rather than http.DefaultServeMux via | ||
| // http.HandleFunc/http.ListenAndServe) so this experimental endpoint never | ||
| // accidentally exposes handlers registered on the default mux by other | ||
| // packages (e.g. net/http/pprof, which is blank-imported for the separate, | ||
| // opt-in -enable-pprof server). | ||
| func (s *Server) Start() error { | ||
| http.HandleFunc("/api/concurrency", s.handleConcurrency) | ||
| if err := http.ListenAndServe(s.addr, nil); err != nil { | ||
| if s.token == "" { | ||
| gologger.Warning().Msgf("http-api-endpoint is running without a token (-http-api-token); anyone able to reach %s can read and change scan settings", s.addr) | ||
| } | ||
|
|
||
| mux := http.NewServeMux() | ||
| mux.HandleFunc("/api/concurrency", s.handleConcurrency) | ||
|
|
||
| var handler http.Handler = mux | ||
| if s.token != "" { | ||
| handler = s.tokenAuthMiddleware(handler) | ||
| } | ||
|
|
||
| server := &http.Server{ | ||
| Addr: s.addr, | ||
| Handler: handler, | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| ReadTimeout: 10 * time.Second, | ||
| WriteTimeout: 10 * time.Second, | ||
| IdleTimeout: 60 * time.Second, | ||
| } | ||
| if err := server.ListenAndServe(); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // tokenAuthMiddleware requires a matching ?token= query parameter on every | ||
| // request when a token has been configured. | ||
| func (s *Server) tokenAuthMiddleware(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| token := r.URL.Query().Get("token") | ||
| if token == "" || token != s.token { | ||
| http.Error(w, "Unauthorized", http.StatusUnauthorized) | ||
| return | ||
| } | ||
| next.ServeHTTP(w, r) | ||
| }) | ||
| } | ||
|
Comment on lines
+73
to
+84
Contributor
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. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Harden Two issues affect this authentication check:
Use 🔒 Proposed fix for constant-time comparison and header-based token+import (
+ "crypto/subtle"
+ "strings"
+)
+
func (s *Server) tokenAuthMiddleware(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- token := r.URL.Query().Get("token")
- if token == "" || token != s.token {
+ token := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ")
+ if token == "" {
+ token = r.URL.Query().Get("token")
+ }
+ if token == "" || subtle.ConstantTimeCompare([]byte(token), []byte(s.token)) != 1 {
http.Error(w, "Unauthorized", http.StatusUnauthorized)
return
}
next.ServeHTTP(w, r)
})
}🤖 Prompt for AI Agents |
||
|
|
||
| // handleConcurrency routes the request based on its method to the appropriate handler. | ||
| func (s *Server) handleConcurrency(w http.ResponseWriter, r *http.Request) { | ||
| switch r.Method { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.