Skip to content

Fix /machine/map endpoint vulnerability #2642

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.

Sign up for GitHub

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

Merged
kradalby merged 6 commits into from
Jun 6, 2025
Merged

Fix /machine/map endpoint vulnerability #2642

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ae30020
Improve map auth logic
Thifhi Jun 5, 2025
7610e8c
Bugfix
Thifhi Jun 5, 2025
fe97600
Add comment, improve error message
Thifhi Jun 5, 2025
e03ec31
noise: make func, get by node
kradalby Jun 6, 2025
84337b5
noise: add comment to remind future use of getAndVal
kradalby Jun 6, 2025
339a9cf
changelog: add entry
kradalby Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions hscontrol/noise.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,8 @@ func (ns *noiseServer) NoisePollNetMapHandler(
return
}

ns.nodeKey = mapRequest.NodeKey
node, err := ns.headscale.db.GetNodeByMachineKey(ns.machineKey)

node, err := ns.headscale.db.GetNodeByNodeKey(mapRequest.NodeKey)
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
httpError(writer, NewHTTPError(http.StatusNotFound, "node not found", nil))
Expand All @@ -221,6 +220,13 @@ func (ns *noiseServer) NoisePollNetMapHandler(
return
}

if ns.nodeKey != mapRequest.NodeKey {
httpError(writer, NewHTTPError(http.StatusNotFound, "node does not belong to machine key", nil))
Copy link

@ghost ghost Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hscontrol/noise.go, the error message 'node does not belong to machine key' on line 224 might be confusing. Consider changing it to something more precise like 'node key in request doesn't match the one associated with this machine key' to clarify the exact validation that's failing.

return
}
Copy link

@ghost ghost Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hscontrol/noise.go, the code now checks that the nodeKey from the map request matches the existing nodeKey before processing. This is a security improvement, but have you considered adding test cases to verify this validation works correctly?


ns.nodeKey = node.NodeKey
Copy link

@ghost ghost Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hscontrol/noise.go, line 228 assigns ns.nodeKey = node.NodeKey after the validation check. However, if ns.nodeKey hasn't been properly initialized before this handler is called, the validation check on line 223 might fail incorrectly. Consider verifying if ns.nodeKey is always properly set before reaching this handler.


sess := ns.headscale.newMapSession(req.Context(), mapRequest, writer, node)
sess.tracef("a node sending a MapRequest with Noise protocol")
if !sess.isStreaming() {
Expand Down