Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add signature verification to V2 tool install endpoint #826

Merged
merged 7 commits into from
Sep 26, 2023

Conversation

umbynos
Copy link
Contributor

@umbynos umbynos commented Sep 7, 2023

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • Tests for the changes have been added (for bug fixes / features)
  • What kind of change does this PR introduce?

security enhancement in the V2 tools install endpoint

  • What is the current behavior?

The "URL" and "Checksum" arguments of the /v2/pkgs/tools/installed endpoint are overriding "name", "packager" and "version" without performing signature verification:

$ curl -X POST --header 'Content-Type: application/json' --d
ata '{"name": "dummy","version": "dummy","packager": "dumm
y", "url": "https://dl.espressif.com/dl/esptool-2.3.1-linu
x.tar.gz","checksum": "SHA-256:cff30841dad80ed5d7d2d58a318
43b63afa57528979a9c839806568167691d8e"}' 127.0.0.1:8991/v2
/pkgs/tools/installed

{"status":"ok"}

$ ll ~/.arduino-create/dummy/dummy/dummy/esptool.py 
-rw-r--r-- 1 umberto umberto 115K Sep  7 12:26 /home/umberto/.arduino-create/dummy/dummy/dummy/esptool.py
  • What is the new behavior?

If "URL" or "Checksum" or "Signature" arguments are not present, we try to install the tool from the loaded package_index.json:
With the same CURL listed above, this is the result:

{"name":"not_found","id":"IoHH-Xxv","message":"tool not found with packager 'dummy', name 'dummy', version 'dummy'","temporary":false,"timeout":false,"fault":false}

$ ll ~/.arduino-create/dummy/dummy/dummy/esptool.py
ls: cannot access '/home/umberto/.arduino-create/dummy/dummy/dummy/esptool.py': No such file or directory

The signature is used to verify the URL.
If the signature is present but does not match:

$ curl -X POST --header 'Content-Type: application/json' --data '{"name": "dummy","version": "dummy","packager": "dummy", "url": "https://dl.espressif.com/dl/esptool-2.3.1-linux.tar.gz","checksum": "SHA-256:cff30841dad80ed5d7d2d58a31843b63afa57528979a9c839806568167691d8e", "signature": "wr0ngS1gn4tur3"}' 127.0.0.1:8991/v2/pkgs/tools/installed

{"name":"fault","id":"5sZKrQPL","message":"crypto/rsa: verification error","temporary":false,"timeout":false,"fault":true}

ll ~/.arduino-create/dummy/dummy/dummy/esptool.py
ls: cannot access '/home/umberto/.arduino-create/dummy/dummy/dummy/esptool.py': No such file or directory
  • Does this PR introduce a breaking change?

Technically no, since the Web Editor is already sending the "signature" argument in the request to the agent.

  • Other information:

@umbynos umbynos added type: enhancement Proposed improvement topic: code Related to content of the project itself topic: security Related to the protection of user data labels Sep 7, 2023
@umbynos umbynos self-assigned this Sep 7, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 67.74% and project coverage change: +0.44% 🎉

Comparison is base (08422e6) 16.88% compared to head (b7d18c2) 17.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
+ Coverage   16.88%   17.32%   +0.44%     
==========================================
  Files          53       53              
  Lines        4075     4081       +6     
==========================================
+ Hits          688      707      +19     
+ Misses       3287     3273      -14     
- Partials      100      101       +1     
Flag Coverage Δ
unit 17.32% <67.74%> (+0.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
conn.go 0.00% <0.00%> (ø)
gen/tools/service.go 18.51% <ø> (ø)
main.go 2.28% <ø> (ø)
utilities/utilities.go 18.62% <60.00%> (+7.13%) ⬆️
gen/http/tools/server/types.go 36.73% <66.66%> (+0.56%) ⬆️
v2/pkgs/tools.go 67.30% <100.00%> (+0.86%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The endpoint affected is `/v2/pkgs/tools/installed`.
If the signature is invalid the endpoint returns 500 with "rsa verification error"
If the signature is not present we try to install the tool using "name, version, packager" arguments
@umbynos umbynos force-pushed the add-signature-tool-install branch from b7d18c2 to a839105 Compare September 25, 2023 14:39
@umbynos umbynos merged commit 33080c3 into main Sep 26, 2023
@umbynos umbynos deleted the add-signature-tool-install branch September 26, 2023 07:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
topic: code Related to content of the project itself topic: security Related to the protection of user data type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants