Skip to content
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

Enhance signature algorithm for code signatures #24727

Closed
wants to merge 5 commits into from

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Dec 16, 2020

No description provided.

This was referenced Dec 18, 2020
This was referenced Dec 28, 2020
@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 8, 2021
@nickvergessen
Copy link
Member

Setting to 22, otherwise already published apps will fail hard on install

@MorrisJobke

This comment has been minimized.

rullzer and others added 5 commits January 8, 2021 15:02
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
But also check it if the new signatures are there

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen

This comment has been minimized.

@nickvergessen
Copy link
Member

Conflicts and 22, setting back to developing

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 20, 2021
@rullzer

This comment has been minimized.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Seems to work, so approving in advance


if ($forceHash && isset($signatureData['signatures'][$forceHash])) {
// Check the sha512 hash
$rsa->setHash($forceHash);
Copy link
Member

Choose a reason for hiding this comment

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

That scares me a bit because it could eventually lead to something like https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/, in which there is a null algorithm or something eventually.

Can we instead pass a version identifier (e.g. 2 == SHA512)?

$minVersion = $this->infoParser->getMinVersion($path . '/appinfo/info.xml');
$forceHash = null;
if ($minVersion >= 22) {
$forceHash = 'sha512';
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate the reason for the hardcoding here? :)

@LukasReschke LukasReschke self-requested a review May 11, 2021 12:40
This was referenced May 20, 2021
@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@skjnldsv
Copy link
Member

skjnldsv commented Jun 2, 2021

@LukasReschke do you want/can to take over?

Or else we move for 23 :)

@LukasReschke LukasReschke removed this from the Nextcloud 22 milestone Jun 9, 2021
@LukasReschke
Copy link
Member

-> 23. I feel it's a bit late for this change now.

@juliushaertl juliushaertl removed their request for review April 5, 2023 14:17
@skjnldsv skjnldsv removed the 2. developing Work in progress label Feb 27, 2024
@skjnldsv
Copy link
Member

As there is no feedback since a while I will close this ticket.
If you will decide to work on this feature again and if it hasn't been fixed or implemented already, feel free to re-open and solve the various conflicts.

@skjnldsv skjnldsv closed this Feb 27, 2024
@skjnldsv skjnldsv deleted the enh/signature_alg branch February 27, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants