Fix `/machine/map` endpoint vulnerability #2642

Thifhi · 2025-06-05T12:11:24Z

Current Behavior

POST /machine/map requests are processed by extracting the Node from the public NodeKey provided in the request, without any additional verification. This allows an attacker who possesses the public key of any Node in the network to:

Establish a Noise connection using a new machine key,
Submit a MapRequest containing a NodeKey that does not belong to them, and
Receive a stream of MapResponses.

As a result, the attacker can learn sensitive details about the entire Tailnet.

Problem

The current approach implicitly trusts the NodeKey in the request, even though the client has not proven ownership of its corresponding private key.

Proposed Change

Instead, we should retrieve the Node based on the machine key used in the Noise connection, as the client must possess the corresponding private key to establish the session. This provides proper authentication and matches the behavior of Tailscale servers. This is what Tailscale control plane returns in this case:

have read the CONTRIBUTING.md file
raised a GitHub issue or discussed it on the projects chat beforehand
added unit tests
added integration tests
updated documentation if needed
updated CHANGELOG.md

ghost · 2025-06-05T12:11:28Z

Pull Request Revisions

Revision	Description
r5	Node connection key matching improved Updated node connection logic to validate both node and machine keys simultaneously
r4	Added node validation for noise Introduced `getAndValidateNode` method to centralize node retrieval and machine key validation for Noise protocol handlers
r3	Improved node key validation logic Enhanced node key validation in noise polling handler with more descriptive error message and clearer node key matching check
r2	Node key comparison field changed Modified node key comparison from `ns.nodeKey` to `node.NodeKey` in noise map request handler
r1	Node key verification and retrieval updated Modified node lookup logic to first retrieve node by machine key, then validate node key match

✅ AI review completed for r5

Help

React with emojis to give feedback on AI-generated reviews:

👍 means the feedback was helpful and actionable
👎 means the feedback was incorrect or unhelpful

💬 Replying to feedback with a comment helps us improve the system. Your input also contributes to shaping future interactions with the AI reviewer.

We'd love to hear from you—reach out anytime at team@review.ai.

ghost · 2025-06-05T12:12:12Z

hscontrol/noise.go

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


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?

ghost · 2025-06-05T12:12:12Z

hscontrol/noise.go

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


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.

ghost · 2025-06-05T12:12:12Z

hscontrol/noise.go

 	}

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


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.

ghost · 2025-06-05T12:15:41Z

hscontrol/noise.go

+	if node.NodeKey != mapRequest.NodeKey {
+		httpError(writer, NewHTTPError(http.StatusNotFound, "node does not belong to machine key", nil))
+		return
+	}


In hscontrol/noise.go, consider adding a comment explaining the security implications of this change. The new approach correctly verifies that the node key in the map request matches the one associated with the machine key before proceeding, which prevents potential spoofing attacks.

kradalby

Good catch!, thank you. I think the review agent has two good points, can you address those?

ghost · 2025-06-05T12:42:01Z

hscontrol/noise.go

 				return &regReq, resp
 			} else {
 			}


In hscontrol/noise.go, there's an empty else block at lines 275-277. Consider removing the empty block for cleaner code.

Thifhi · 2025-06-05T12:42:59Z

Good catch!, thank you. I think the review agent has two good points, can you address those?

Addressed 👍

kradalby · 2025-06-05T12:49:47Z

Thank you, i'll merge after the tests

Thifhi · 2025-06-05T14:04:19Z

Also I just realized that MapRequest is used to submit updates for the node as well, so just by knowing the node's public key, an attacker might be able to modify the network configuration. I'm not fully familiar with the broader codebase, so I'll leave it to you to assess the severity of this.

kradalby · 2025-06-05T18:12:19Z

I'll take a look tomorrow, also the failing tests

this commit splits the additional validation into a separate function so it can be reused if we add more endpoints in the future. It swaps the check, so we still look up by NodeKey, but before accepting the connection, we validate the known machinekey from the db against the noise connection. The reason for this is that when a node logs in or out, the node key is replaced and it will no longer be possible to look it up, breaking reauthentication. Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

kradalby · 2025-06-06T06:41:53Z

Also I just realized that MapRequest is used to submit updates for the node as well, so just by knowing the node's public key, an attacker might be able to modify the network configuration. I'm not fully familiar with the broader codebase, so I'll leave it to you to assess the severity of this.

This should not be a problem as it happens in the same endpoint as we just added the check. So any update mechanism will not start a stream nor post.

I have just pushed two changes to address some test failures and comments for future. Please take a look @Thifhi, thanks.

Thifhi · 2025-06-06T09:18:51Z

Looks good , thank you

Thifhi · 2025-06-06T09:20:58Z

Though still failing 😕

kradalby · 2025-06-06T09:51:32Z

Though still failing 😕

I think it is only github actions having a bad time, im gonna pull it down and run the ones locally.

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* Improve map auth logic * Bugfix * Add comment, improve error message * noise: make func, get by node this commit splits the additional validation into a separate function so it can be reused if we add more endpoints in the future. It swaps the check, so we still look up by NodeKey, but before accepting the connection, we validate the known machinekey from the db against the noise connection. The reason for this is that when a node logs in or out, the node key is replaced and it will no longer be possible to look it up, breaking reauthentication. Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> Co-authored-by: Kristoffer Dalby <kristoffer@tailscale.com>

See juanfont/headscale#2642

* Improve map auth logic * Bugfix * Add comment, improve error message * noise: make func, get by node this commit splits the additional validation into a separate function so it can be reused if we add more endpoints in the future. It swaps the check, so we still look up by NodeKey, but before accepting the connection, we validate the known machinekey from the db against the noise connection. The reason for this is that when a node logs in or out, the node key is replaced and it will no longer be possible to look it up, breaking reauthentication. Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> Co-authored-by: Kristoffer Dalby <kristoffer@tailscale.com>

Improve map auth logic

ae30020

Thifhi requested review from juanfont and kradalby as code owners June 5, 2025 12:11

ghost approved these changes Jun 5, 2025

View reviewed changes

Bugfix

7610e8c

ghost approved these changes Jun 5, 2025

View reviewed changes

kradalby approved these changes Jun 5, 2025

View reviewed changes

Add comment, improve error message

fe97600

ghost approved these changes Jun 5, 2025

View reviewed changes

kradalby added 2 commits June 6, 2025 08:35

noise: add comment to remind future use of getAndVal

84337b5

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

ghost approved these changes Jun 6, 2025

View reviewed changes

changelog: add entry

339a9cf

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

kradalby requested review from nblock and ohdearaugustin as code owners June 6, 2025 10:00

ghost approved these changes Jun 6, 2025

View reviewed changes

nblock approved these changes Jun 6, 2025

View reviewed changes

kradalby merged commit bad7833 into juanfont:main Jun 6, 2025
140 of 150 checks passed

EdGeraghty pushed a commit to privacyint/docker-headscale that referenced this pull request Jun 6, 2025

Bump headscale for security fix

106660b

See juanfont/headscale#2642

EdGeraghty mentioned this pull request Jun 6, 2025

Bump headscale for security fix privacyint/docker-headscale#52

Merged

phaer mentioned this pull request Jun 6, 2025

headscale: 0.26.0 -> 0.26.1 NixOS/nixpkgs#414539

Merged

13 tasks

Uh oh!

Fix /machine/map endpoint vulnerability #2642

Fix /machine/map endpoint vulnerability #2642

Uh oh!

Conversation

Thifhi commented Jun 5, 2025

Current Behavior

Problem

Proposed Change

Uh oh!

ghost commented Jun 5, 2025 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Revisions

Uh oh!

ghost Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

kradalby left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Thifhi commented Jun 5, 2025

Uh oh!

kradalby commented Jun 5, 2025

Uh oh!

Thifhi commented Jun 5, 2025

Uh oh!

kradalby commented Jun 5, 2025

Uh oh!

kradalby commented Jun 6, 2025

Uh oh!

Thifhi commented Jun 6, 2025

Uh oh!

Thifhi commented Jun 6, 2025

Uh oh!

kradalby commented Jun 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix `/machine/map` endpoint vulnerability #2642

Fix `/machine/map` endpoint vulnerability #2642

ghost commented Jun 5, 2025 •

edited by ghost

Loading