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

desktop: Check OpenH264 library downloads with SHA256 instead of MD5 #17243

Closed
wants to merge 0 commits into from

Conversation

Fullmetal5
Copy link
Contributor

Use SHA256 instead of MD5 for checking integrity of downloaded file, especially since this is over HTTP and not HTTPS.

@evilpie
Copy link
Collaborator

evilpie commented Jul 23, 2024

Where do these hashes come from? As far as I can tell cisco sadly only published md5 hashes for the releases: https://github.com/cisco/openh264/releases/tag/v2.4.1.

@Fullmetal5
Copy link
Contributor Author

Fullmetal5 commented Jul 23, 2024

Where do these hashes come from? As far as I can tell cisco sadly only published md5 hashes for the releases: https://github.com/cisco/openh264/releases/tag/v2.4.1.

I actually hadn't noticed that signed MD5's, I thought they were all just library downloads at first glance. I just downloaded each file and calculated it's hash locally.

Since everyone can verify these hashes would it still be a problem changing this to SHA256? It would be the same amount of work in the future to swap these out as the version changes and either way we are trusting the committer and merger.

(I see the lint issue and am fixing it.)

@torokati44
Copy link
Member

As far as I can tell cisco sadly only published md5 hashes for the releases:

Yes, the reason we're checking MD5 is because that's the only verification we're provided with...

And don't get me started on...

Since everyone can verify these hashes would it still be a problem changing this to SHA256?

I don't really see the reason to do so. I mean, I get the theoretical advantages, but I'd prefer checking directly against what Cisco gives us as "signatures".

@torokati44
Copy link
Member

torokati44 commented Jul 23, 2024

So for the record, this gets a "slightly against" vote from me. I'd much prefer if Cisco would finally:

  • Fix their SSL.
  • Provide contemporary hashes.
  • Provide actual signatures instead of mistaking, and misleading everyone, that hashes are that.

(EDIT: But if this gets through anyway, there are some relevant lines in the Cargo.toml of this crate that will no longer be necessary once md-5 gets dropped.)

But anyway, thank you for the effort!

@Fullmetal5
Copy link
Contributor Author

Fullmetal5 commented Jul 23, 2024

As far as I can tell cisco sadly only published md5 hashes for the releases:

Yes, the reason we're checking MD5 is because that's the only verification we're provided with...

And don't get me started on...

Since everyone can verify these hashes would it still be a problem changing this to SHA256?

I don't really see the reason to do so. I mean, I get the theoretical advantages, but I'd prefer checking directly against what Cisco gives us as "signatures".

If you would like to keep it that way then I understand. Annoyingly if Cisco just didn't publish any signatures then we wouldn't have to stick with their bad decision and probably would have made a better choice to begin with.

EDIT: Wow, they aren't even signed, it's literally just the hash hosted on the webserver, now I'm extra disappointed.

Close this PR if you decide against changing it.

@torokati44
Copy link
Member

EDIT: Wow, they aren't even signed, it's literally just the hash hosted on the webserver, now I'm extra disappointed.

Yeah. 🥲 Over plain HTTP. 😭

@torokati44 torokati44 closed this Jul 24, 2024
@torokati44
Copy link
Member

Update: I have just received information from @kjarosh that we wouldn't be the first ones doing exactly this: https://gitlab.com/freedesktop-sdk/openh264-extension/-/blob/d17d101c157097febb6153618460abf5b2aa495f/elements/config.yml

So, this might happen anyway, in some shape, way, or form.

@kjarosh kjarosh reopened this Aug 9, 2024
@kjarosh
Copy link
Member

kjarosh commented Aug 9, 2024

So after a discussion, we've decided that the current HTTP+MD5 is not up to our security standards and we can do better than Cisco.

It's more secure to calculate SHA256 on a safe(r) network than use MD5 everywhere.

I'll be refreshing this PR in the near future.

@kjarosh kjarosh self-assigned this Aug 9, 2024
@Fullmetal5
Copy link
Contributor Author

So after a discussion, we've decided that the current HTTP+MD5 is not up to our security standards and we can do better than Cisco.

It's more secure to calculate SHA256 on a safe(r) network than use MD5 everywhere.

I'll be refreshing this PR in the near future.

Awesome, glad to hear.

@kjarosh
Copy link
Member

kjarosh commented Aug 9, 2024

Whoops, I pushed a wrong branch and GitHub won't allow me to push the right one now... Sorry for messing this up, I'll open a new PR based on your changes @Fullmetal5

@kjarosh
Copy link
Member

kjarosh commented Aug 9, 2024

Continued in #17452

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants