Fixes #35804 by computing the DDT of non-square sboxes properly. #36020

PTK12 · 2023-08-02T14:36:08Z

After some investigation, it was found that many non-square sboxes did not work.
In cases where the output size is bigger than the input size (e.g. SBox(16,16,16,16,16,16,16,16)), an incorrect DDT is produced.
In cases where where input size is bigger, it leads to an IndexError such as in #35804.

di spans from [0, nrows)
When multiplied by nrows, a size of nrows * (nrows - 1) can be achieved which can be greater than the maximum size of the table when nrows > ncols (leading to the behaviour seen earlier).

Changing the multiplication of nrows to ncols fixes the IndexError as well as produce the correct SBox for SBox(16,16,16,16,16,16,16,16).

Tested with doctests along with the following script:

import random
from sage.crypto.sbox import SBox

random.seed(b"sbox")

test = lambda x: print(SBox(x).difference_distribution_table())

# Square sbox
test([7,6,0,4,2,5,1,3])
for _ in range(10):
	sbox = random.sample(range(16), 16)
	print(sbox)
	test(sbox)

# Non-square sboxes
for _ in range(10): # input > output
	sbox = random.sample(list(range(4)) * 4, 16)
	print(sbox)
	test(sbox)

for _ in range(10): # input < output
	sbox = random.sample(range(16), 4)
	print(sbox)
	test(sbox)

print("Passed all cases!")

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

Fixes sagemath#35804

PTK12 · 2023-08-02T14:49:33Z

Could someone add labels for this please? I'm not sure if I'm able to.

kwankyu · 2023-08-04T00:47:59Z

@roed314, workflows need approval.

github-actions · 2023-08-04T06:36:56Z

Documentation preview for this PR (built with commit 59c59ee; changes) is ready! 🎉

PTK12 · 2023-09-17T23:07:32Z

@roed314 Sorry for disturbing you but can I get an update on this PR and any changes it would need? Thanks.

kwankyu

LGTM.

PTK12 · 2023-09-18T18:49:07Z

Thanks! 👍

…tion      Unfortunately the fix in sagemath#36213 has not been completed properly. There is still a `return` from the `get_review_decision` method which is not `None` where it should be. This is fixed here. Sorry for this unnecessary trouble and my lack of attention! In a next step this PR fixes a bug which has been noticed by an accidentally activation of the `unlabeled` trigger (see sagemath#36292 (comment)). Simultanously the `labeled`-trigger has been disabled. This is an unrealistic setting which was not covered by the code. Explicitely, a state label could not be removed even though other state labels where set (examples sagemath#36128 and sagemath#36020). This PR leads to the following changes of behavior: 1. The reaction on the `unlabeled`-event is reduced to the case of the last label 2. The `unlabeled`-event will not lead to a rejection of the removal any more 3. Instead of a warning comment a hint comment is posted. We found a way to test the bot a little more realistically (see soehms#11). Additional problems emerged and were also resolved. This led to the following changes in the workflow: 4. A bug in `actor_valid` is fixed (see soehms#10 (comment)) 5. Preparing to use our own bot user (e.g. [sagemathadmins](https://github.com/sagemathadmins)) (see soehms#11 (comment)) 6. Waiver of observing the `reviewDecision` feature provided by GitHub (see 2. in soehms#11 (comment)) 7. Allow the user to revert his decision of label selection (see soehms#11 (comment)) 8. Don't reject label addition any more, except in the case where the author tries to set `s: positive review` to his own PR which has no reviews from others (see soehms#11 (comment)) 9. Dismiss stale reviews of the bot after a push to the branch and on submission of a new review which is more than just a comment (see soehms#11 (comment)). Despite the fact that the testing was now more realistic, it is still not guaranteed that the bot's behavior in `sagemath/sage` will be completely covered by our testing. The changes in this PR cannot be activated immediately after merging into the develop branch due to a bug in the GitHub web interface observed during testing in soehms#11 (see soehms#11 (comment) and soehms#11 (comment)). The problem is that the panel at the top right of the webpage that contains the labels does not update immediately after the bot changes the labels. Since this might cause confusion, I'll create a bug report about it. The going-live of the sync bot will be stalled at least until we received an answer. This PR changes the following flowcharts from sagemath#35172 : ##### What happens when adding `s: needs review` to a PR? ```mermaid --- title: add the needs review label --- flowchart LR %% vertices trigger([label\n 's: needs review'\n added]) mark_as_ready([mark as\n ready for\n review]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_review_valid[[needs review\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_review_valid needs_review_valid -- true ---> remove_other_labels needs_review_valid -- false ---> is_draft is_draft -- yes ---> mark_as_ready is_draft -- no ---> warning_about_label_addition mark_as_ready --> remove_other_labels ``` The warning is posted as a comment which will be deleted after 5 minutes. ```mermaid --- title: needs review valid? --- flowchart LR %% vertices needs_review_valid([needs review\n valid?]) true([true]) false([false]) latest_review_by_actor{latest review\n by actor} needs_work_valid[[needs work\n valid?]] positive_review_valid[[positive review\n valid?]] is_draft{is PR\n a draft?} %% edges needs_review_valid ---> is_draft is_draft -- yes ---> false is_draft -- no ---> latest_review_by_actor latest_review_by_actor -- yes ---> true latest_review_by_actor -- no ---> needs_work_valid needs_work_valid -- true ---> false needs_work_valid -- false ---> positive_review_valid positive_review_valid -- true ---> false positive_review_valid -- false ---> true ``` ```mermaid --- title: needs work valid? --- flowchart LR %% vertices needs_work_valid([needs work\n valid?]) true([true]) false([false]) latest_review_request_changes{latest proper\n review requests\n changes?} %% edges needs_work_valid --> latest_review_request_changes latest_review_request_changes -- yes ---> true latest_review_request_changes -- no ---> false ``` ```mermaid --- title: positive review valid? --- flowchart LR %% vertices positive_review_valid([positive review\n valid?]) true([true]) false([false]) latest_proper_review_approved{latest proper\n review\n approved?} %% edges positive_review_valid --> latest_proper_review_approved latest_proper_review_approved -- yes ---> true latest_proper_review_approved -- no ---> false ``` Here, proper means that the review is more than a comment. ##### What happens when adding s: needs work to a PR? ```mermaid --- title: add the needs work label --- flowchart LR %% vertices trigger([label\n 's: needs work'\n added]) request_changes([request changes]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_work_valid[[needs work\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_work_valid needs_work_valid -- true ---> remove_other_labels needs_work_valid -- false ---> is_draft is_draft -- yes ---> warning_about_label_addition is_draft -- no ---> request_changes request_changes --> remove_other_labels ``` ##### What happens when adding `s: positive review` to a PR? ```mermaid --- title: add the positive review label --- flowchart LR %% vertices trigger([label\n 's: positive review'\n added]) positive_review_valid[[positive review\n valid?]] approve_pr([approve]) remove_other_labels([remove other\n state labels]) actor_valid[[actor valid?]] approve_allowed[[approve\n allowed?]] warning_about_label_addition([warning\n about label\n addition]) reject_label_addition[[reject\n label\n addition]] %% edges trigger --> positive_review_valid positive_review_valid -- yes ---> remove_other_labels positive_review_valid -- no ---> actor_valid actor_valid -- yes ---> approve_allowed actor_valid -- no ---> reject_label_addition approve_allowed -- yes ---> approve_pr approve_allowed -- no ---> warning_about_label_addition approve_pr --> remove_other_labels ``` The boxes `actor_valid` and `reject_label_addition` are unchanged. ```mermaid --- title: approve allowed? --- flowchart LR %% vertices trigger([approve\n allowed?]) true([true]) false([false]) review_of_others_request_changes{changes\n requested by\n someone\n else exists?} %% edges trigger --> review_of_others_request_changes review_of_others_request_changes -- yes ---> false review_of_others_request_changes -- no ---> true ``` Here, only reviews of someone else are considered which are more recent than any commit. ##### What happens when a state label is added to an issue? ```mermaid --- title: add a state label to an issue --- flowchart LR %% vertices trigger([label added]) warning_about_label_addition([warning\n about label\n addition]) nothing([do nothing]) is_needs_info{is label\n 's: needs info'?} %% edges trigger --> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> warning_about_label_addition ``` ##### What happens when removing a state label from a PR? ```mermaid --- title: remove a state label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_state_label{other state\n labels exist?} is_needs_info{is label\n 's: needs info'?} %% edges trigger --> other_state_label other_state_label -- yes ---> nothing other_state_label -- no ---> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> hint_about_label_removal ``` The hint is postet as a comment which will be deleted after 5 minutes. ##### What happens when adding a priority label? ```mermaid --- title: add a priority label --- flowchart LR %% vertices trigger([label added]) remove_other_labels([remove other\n priority labels]) %% edges trigger --> remove_other_labels ``` ##### What happens when removing a priority label? ```mermaid --- title: remove a priority label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_prio_label{other priority\n labels exist?} %% edges trigger --> other_prio_label other_prio_label -- yes ---> nothing other_prio_label -- no ---> hint_about_label_removal ``` ##### What happens when a PR is approved? ```mermaid --- title: approve --- flowchart LR %% vertices trigger([approve\n]) select_positive_review(['s: positive review'\n label selected]) nothing([do nothing]) positive_review_valid[[positive review\n valid?]] pending_review_requests{pending\n review requests\n exists?} actor_authorized{is actor\n a member of\n Triage?} %% edges trigger ---> pending_review_requests pending_review_requests -- yes ---> nothing pending_review_requests -- no ---> actor_authorized actor_authorized -- yes ---> positive_review_valid actor_authorized -- no ---> nothing positive_review_valid -- true ---> select_positive_review positive_review_valid -- false ---> nothing ``` ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#36292 Reported by: Sebastian Oehms Reviewer(s): Kwankyu Lee

Change nrows to ncols

e594552

Fixes sagemath#35804

Updated docstrings and included doctests

59c59ee

PTK12 changed the title ~~Allow computing DDT of non-square sboxes.~~ Fixes #35804 by computing the DDT of non-square sboxes properly. Aug 3, 2023

kwankyu added t: enhancement p: minor / 4 s: needs review c: cryptography labels Aug 4, 2023

Merge branch 'develop' into develop

1a9f82d

kwankyu approved these changes Sep 18, 2023

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Sep 18, 2023

github-actions bot added the s: needs review label Sep 18, 2023

kwankyu removed the s: needs review label Sep 18, 2023

github-actions bot added the s: needs review label Sep 18, 2023

soehms mentioned this pull request Sep 18, 2023

Fix sync labels issues for step 2 going live #36213

Merged

5 tasks

kwankyu removed the s: needs review label Sep 18, 2023

soehms mentioned this pull request Sep 19, 2023

Fix method on_label_removal soehms/sage#10

Merged

5 tasks

vbraun merged commit f9541a4 into sagemath:develop Sep 24, 2023
6 checks passed

github-actions bot removed the s: positive review label Sep 24, 2023

mkoeppe added this to the sage-10.2 milestone Sep 24, 2023

soehms approved these changes Sep 26, 2023

View reviewed changes

soehms mentioned this pull request Oct 19, 2023

Fix sync labels issues for step 2 going live completion #36292

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #35804 by computing the DDT of non-square sboxes properly. #36020

Fixes #35804 by computing the DDT of non-square sboxes properly. #36020

PTK12 commented Aug 2, 2023 •

edited

Loading

PTK12 commented Aug 2, 2023

kwankyu commented Aug 4, 2023

github-actions bot commented Aug 4, 2023

PTK12 commented Sep 17, 2023

kwankyu left a comment

PTK12 commented Sep 18, 2023

Fixes #35804 by computing the DDT of non-square sboxes properly. #36020

Fixes #35804 by computing the DDT of non-square sboxes properly. #36020

Conversation

PTK12 commented Aug 2, 2023 • edited Loading

📝 Checklist

⌛ Dependencies

PTK12 commented Aug 2, 2023

kwankyu commented Aug 4, 2023

github-actions bot commented Aug 4, 2023

PTK12 commented Sep 17, 2023

kwankyu left a comment

Choose a reason for hiding this comment

PTK12 commented Sep 18, 2023

PTK12 commented Aug 2, 2023 •

edited

Loading