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

fix: add a compatibility EVP_CIPH_OCB_MODE value (#16214). #17873

Merged
merged 3 commits into from Apr 23, 2019

Conversation

Projects
None yet
4 participants
@TaikiAkita
Copy link

commented Apr 19, 2019

Backport of google/boringssl@4b9683. This patch replaces the no-op modes with negative numbers rather than zero. Stream ciphers like RC4 report a "mode" of zero, so code comparing the mode to a dummy value will get confused.

This patch fixed issue #16214.

Release Notes

Notes: Fixed the "rc4" cipher (#16214) (4.1.x).

fix: add a compatibility EVP_CIPH_OCB_MODE value (#16214).
Backported google/boringssl@4b9683 to 4.1.x branch. This patch
replaces the no-op modes with negative numbers rather than zero.
Stream ciphers like RC4 report a "mode" of zero, so code comparing
the mode to a dummy value will get confused.

This patch fixed issue #16214.
@@ -1,3 +1,4 @@
compatibility_evp_ciph_ocb_mode.patch

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Apr 19, 2019

Member

Was this patch added manually? I'm not sure how this could have been added to the top of the file if the export-patches script was used

This comment has been minimized.

Copy link
@TaikiAkita

TaikiAkita Apr 19, 2019

Author

Was this patch added manually? I'm not sure how this could have been added to the top of the file if the export-patches script was used

This patch was added manually. Shall I re-add it with the export-patches command?

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Apr 19, 2019

Member

@TaikiAkita Yep, the flow for making these patches is like this

  • Run gclient sync --your --flags --here
  • Add your commit on top of the checked out boringssl
  • Run git-export-patches -o electron/patches/common/boringssl

👍

This comment has been minimized.

Copy link
@TaikiAkita

TaikiAkita Apr 19, 2019

Author

@MarshallOfSound

I regenerated the patches with git-export-patches, please have a look.

TaikiAkita added some commits Apr 19, 2019

fix: (recommit) add a compatibility EVP_CIPH_OCB_MODE value (#16214).
Recommited change @f1156a with patches generated by
"git-export-patches".
@TaikiAkita

This comment has been minimized.

Copy link
Author

commented Apr 19, 2019

@MarshallOfSound

I regenerated the patches with git-export-patches, please have a look.

@codebytere
Copy link
Member

left a comment

LGTM after changes but paging @MarshallOfSound for second opinion!

@MarshallOfSound
Copy link
Member

left a comment

This patch is currently flagged as an invalid backport because it is targeting 4.1.

Can you confirm that this fix is not needed in current 5.0 or master branches?

@TaikiAkita

This comment has been minimized.

Copy link
Author

commented Apr 19, 2019

This patch is currently flagged as an invalid backport because it is targeting 4.1.

Can you confirm that this fix is not needed in current 5.0 or master branches?

@MarshallOfSound

This patch is exactly backported from 5.x. The upstream BoringSSL of 5.x branch has this patch already. So this patch is NOT needed in 5.x.

Test with following code:

var cipher = require("crypto").createCipher("rc4", "hello");

In 4.x, an error will occur.
In 5.x, run successfully.

BTW. This patch should also be backported to 4.0.x branch.

@TaikiAkita

This comment has been minimized.

Copy link
Author

commented Apr 20, 2019

@codebytere
Now it is ready to be merged. Please help to merge this PR.

@electron-cation electron-cation bot removed the new-pr 🌱 label Apr 20, 2019

@TaikiAkita

This comment has been minimized.

Copy link
Author

commented Apr 23, 2019

@codebytere
Please merge this PR.

@shiftkey

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@TaikiAkita

This comment has been minimized.

Copy link
Author

commented Apr 23, 2019

This patch is currently flagged as an invalid backport because it is targeting 4.1.
Can you confirm that this fix is not needed in current 5.0 or master branches?

@MarshallOfSound

This patch is exactly backported from 5.x. The upstream BoringSSL of 5.x branch has this patch already. So this patch is NOT needed in 5.x.

Test with following code:

var cipher = require("crypto").createCipher("rc4", "hello");

In 4.x, an error will occur.
In 5.x, run successfully.

BTW. This patch should also be backported to 4.0.x branch.

@shiftkey
Described above. This patch was derived from upstream and can only be applied in 4.x branches, it was NOT derived from other PR (so I can not add the "Backport of #{N}" declaration).

@codebytere codebytere merged commit eccede0 into electron:4-1-x Apr 23, 2019

8 of 9 checks passed

Valid Backport Invalid Backport
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Apr 23, 2019

Release Notes Persisted

Fixed the "rc4" cipher (#16214).

@welcome

This comment has been minimized.

Copy link

commented Apr 23, 2019

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.