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

[FHEContext] Fix the divide by zero error #170

Closed
wants to merge 1 commit into from

Conversation

bryonglodencissp
Copy link

🐛 label: CWE-369

Greetings,

In long FindM(long k, long L, long c, long p, long d, long s, long chosen_m, bool verbose), a call to multOrd on line 108 of FHEContext.cpp may assign a value of zero to a divisor of type long. A divide by zero creates an error. It is good practice to validate the value used as a denominator to ensure that a divide by zero error will not cause unexpected results.

Signed-off-by: Bryon Gloden, CISSP® cissp@bryongloden.com

Copy link
Collaborator

@shaih shaih left a comment

Choose a reason for hiding this comment

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

multOrd cannot return zero when GCD(p, ms[i][1]) == 1, which is the condition that is checked above. What parameters were you using to get this error condition?

@bryonglodencissp
Copy link
Author

Owing to mult0rd's comment, "return multiplicative order of p modulo m, or 0 if GCD(p, m) != 1," and its line of code, "(GCD(p, m) != 1) return 0;," we found this bug through manual code review. Furthermore, we are not aware of specific parameters passed that will cause mult0rd to return zero in this context. Would or wouldn't you say a possibility exists for mult0rd to return zero here?

@fionser
Copy link
Contributor

fionser commented Oct 26, 2017

@bryongloden I think using p = 2 and m = 2^K for any power K should cause GCD(p, m) != 1.

@shaih
Copy link
Collaborator

shaih commented Oct 26, 2017

Here is the code:

for (i=0; i<sizeof(ms)/sizeof(long[4]); i++) { 
  if (ms[i][0] < N || GCD(p, ms[i][1]) != 1) continue;
  long ordP = multOrd(p, ms[i][1]);
  [...]

If GCD(p, ms[i][1])!=1 then you hit the continue statement, so you never get to the next line. If GCD(p, ms[i][1])==1 then the multiplicative order is not zero. So I would go with no, I do not think that the possibility exists for multOrd to return zero here.

That said, I'm willing to buy the argument that one should not have to look into multOrd to see what it 's doing, and putting an assertion there is good practice even if it can never be triggered. So I approved the merge request.

@shaih shaih closed this Oct 26, 2017
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.

3 participants