Skip to content
This repository has been archived by the owner on Mar 12, 2024. It is now read-only.

SKI and AKI need to be considered user input #33

Closed
matgnt opened this issue Apr 18, 2023 · 3 comments · Fixed by #53
Closed

SKI and AKI need to be considered user input #33

matgnt opened this issue Apr 18, 2023 · 3 comments · Fixed by #53

Comments

@matgnt
Copy link

matgnt commented Apr 18, 2023

Hi team,

as part of a risk analysis of the DAPS server (see link for details)
Fraunhofer-AISEC/omejdn-server#74

I found out, that SKI and AKI are loaded from the extension of the certificate. In case of self-signed certificates, this needs to be considered user input and should not be trusted directly.
https://github.com/eclipse-tractusx/daps-registration-service/blob/main/src/main/java/org/eclipse/tractusx/dapsreg/util/Certutil.java#L51

Flow

registration_service_client_id_attack_scenario

Since the clientId is not a secret and can be gathered from any EDC interaction (it is part of the DAPS token), this is not a secret. It could happen, that someone elses account / certificate is overwritten and this other party does not get a valid DAPS token any more.

I did not check all the portal checks which happen before the registration service is called. It is good though, that there are checks as well.


Matthias Binzer
Robert Bosch GmbH

@dvasunin
Copy link
Contributor

We re-create SKI using public key from the certificate and compare it with SKI bundled. If they differ we deny request

@matgnt
Copy link
Author

matgnt commented May 12, 2023

The fix mentioned by @dvasunin seems to be part of PR #46

@SebastianBezold
Copy link
Contributor

#46 did indeed contain multiple fixes, but from commit messages it was not really clear, which change fixed what issue.
#50, #51 and #52 have been merged to fix the issues that are not related to this one here.
@dvasunin, will there be another dedicated PR to address the SKI topic?

dvasunin added a commit to catenax-ng/tx-daps-registration-service that referenced this issue May 12, 2023
…tains correct value before creating new registration
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants