1293334 - (CVE-2016-9074) Replace unreliable divSpoiler (timing side-channel defense)

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

As agl notes in [0] the divSpoiler hack to avoid CPU optimisation and thus timing side channels isn't working. We have the same code [1] that we should replace with something similar (Barrett reduction sounds like a good choice).

[0] https://github.com/openssl/openssl/pull/1027
[1] http://searchfox.org/nss/source/lib/ssl/ssl3con.c#13102

Comment 1

[Kai Engert [:KaiE:]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #0)
> [1] http://searchfox.org/nss/source/lib/ssl/ssl3con.c#13102

As a general suggestion, I recommend to mention affected function names, as links with line numbers, that don't refer to a specific revision, will get outdated very quickly.

Comment 2

[Franziskus Kiefer [:franziskus]]

my bad, here the permalink for [1] from comment 1 http://searchfox.org/nss/rev/0c9572bf1d038dc933133836e0f09d25a7684881/lib/ssl/ssl3con.c#13102

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: divSpoil.patch

this is basically the solution from open/boringssl. I'm sure agl doesn't mind, but we can ask him.

Comment 4

[Eric Rescorla (:ekr)]

Franziskus, can you ask Watson?

Comment 5

[Watson Ladd]

Comment on attachment 8779342 [details] [diff] [review]
divSpoil.patch

Review of attachment 8779342 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good: I've verified that the ranges are sufficient for removing the padding lengths we need to deal with, and that they are right.

Comment 6

[Andrew McCreight [:mccr8]]

What should the security rating on this be?
https://wiki.mozilla.org/Security_Severity_Ratings

Comment 7

[Franziskus Kiefer [:franziskus]]

Considering that the original bug 822365 that was thought to fix this issue is sec-high, this one should be sec-high as well.

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8779342 [details] [diff] [review]
divSpoil.patch

Richard can you have a look at this?

Comment 9

[Richard Barnes [:rbarnes]]

Comment on attachment 8779342 [details] [diff] [review]
divSpoil.patch

Review of attachment 8779342 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the documentation comments below addressed.

::: lib/ssl/ssl3con.c
@@ +13102,5 @@
> +
> +    /* The time to compute (macStart - scanStart) % macSize varies based on
> +     * the amount of padding. Thus we explicitely handle all mac sizes with
> +     * (hopefully) constant time modulo.
> +     */

It would be helpful to note the general formula here, e.g.:

/*
 * rotateOffset must be smaller than 1/e, where e is the error in
 * the approximation of 1/macSize by a fraction whose denominator
 * is a power of two.
 */

How do we know that this is the case?  It doesn't seem to follow from the inequalities at the top of the method.  It looks like Watson agrees that we're OK here, but we should describe that here so that people don't have to puzzle over it in the future.

@@ +13148,5 @@
>      /* Now rotate the MAC. If we knew that the MAC fit into a CPU cache line
>       * we could line-align |rotatedMac| and rotate in place. */
>      memset(out, 0, macSize);
> +    rotateOffset = macSize - rotateOffset;
> +    rotateOffset &= ~ssl_ConstantTimeGE(rotateOffset, macSize);

This notation is pretty opaque.  It's equivalent to an ssl_constantTimeSelect, right?  If so, that would be clearer.

Comment 10

[Franziskus Kiefer [:franziskus]]

Attachment: divSpoil.patch (OK for 3.26 branch)

Comment 11

[Kai Engert [:KaiE:]]

Which Firefox branches need this fix?

Comment 12

[Franziskus Kiefer [:franziskus]]

This bug fixes what ought to be fixed in bug 822365. So the exploit is known for about 4 years already. Though the code changed considerably as a result of bug 822365 such that it's not immediately clear to me if/how the original attack still works. It'll land with 3.27 and and such be in Firefox 51.
Dan, do we want to backport the fix to other Firefox versions? I think an uplift to 50 would be reasonable. Not sure about 49 and ESR.

Comment 13

[Daniel Veditz [:dveditz]]

This looks like a small part of the fix in bug 822365. Does this flawed timing attack defense (which seemed to work OK at the time) really defeat the whole of the Lucky13 defense? I'm guessing sec-moderate is more appropriate, but if this does resurrect Lucky13 then we should go back to sec-high.

If it's sec-high we should definitely push for Firefox 49 (NSS 3.25?) -- we still have a week or two. If it's sec-moderate and we're still mostly safe from lucky13 then 50 is probably OK. Either way we'll want this on the ESR release that matches when it ships.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 8783517 [details] [diff] [review]
divSpoil.patch (OK for 3.26 branch)

sec-approval=dveditz

Comment 15

[Gerry Chang [:gchang]]

Track for 49+ to monitor if it will become sec-high.

Comment 16

[Franziskus Kiefer [:franziskus]]

I think sec-moderate is fine, but let's wait for opinions from Wan-Teh and Bob who oversaw the original lucky13 fix.

Comment 17

[Wan-Teh Chang]

I agree sec-moderate is appropriate.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

I think waiting for 50 will be best here. We only have a week to go till release and have a lot of other uncertainties still in play.

Comment 19

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/1e202f0a01b9

Comment 20

[Kai Engert [:KaiE:]]

Based on comments from Dan and Liz, it seems the request is to fix this 
for Firefox 50, and ESR 45.5, scheduled for 2016-11-08

Firefox 50 will require a backport of the fix to NSS_3_26_BRANCH and a NSS 3.26.1 release.

ESR 45.5 will require a backport of the fix to NSS_3_21_BRANCH and a NSS 3.21.2 release.

Franziskus, is the patch compatible with those older branches (in other words, your patch doesn't depend on anything else that has been added after the release of NSS 3.21.1) ?

Comment 21

[Kai Engert [:KaiE:]]

Regarding branch landing timing, I suggest that we land the updated NSS branch releases into the mozilla-beta and mozilla-esr45 branches, shortly after 2016-09-13.

Dan, Liz, does that work for you? Will be give branch commit approvals by mid september?

Comment 22

[Franziskus Kiefer [:franziskus]]

The patch is self-contained. I don't think there's a problem with applying this to 3.26 and 3.21.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

We aren't 100% sure of the release date right now. Probably 2016-09-13, but we have a little uncertainty right now that we may need to slip the date.  Landing after we release should be ok. Dan, what do you think?

Ritu: fyi, here's the NSS update plans for 50 while it is in beta.

Comment 24

[Daniel Veditz [:dveditz]]

sounds good.

Comment 25

[Kai Engert [:KaiE:]]

Attachment: backport to NSS 3.21, v1

Merge attempt for NSS 3.21 branch.

I was testing on Fedora 24 with gcc 6.1.1, and I ran into a warning-as-error, misleading indentation.

I suggest to disable that error on the old branch, which doesn't have our code reformatting yet.

Comment 26

[Kai Engert [:KaiE:]]

Comment on attachment 8783517 [details] [diff] [review]
divSpoil.patch (OK for 3.26 branch)

Your original patch applies fine and builds without errors on NSS_3_26_BRANCH

Comment 27

[Kai Engert [:KaiE:]]

Regarding the announcement of the NSS 3.21.2 and NSS 3.26.1 releases:

When are we allowed to send the public announcement message for those NSS security fix releases?

I'd like to land the code into Firefox beta and ESR 45 as soon as possible (soon after the mit-september releases are done), and this will require me to make the new NSS releases publicly available on FTP/S3 (which means people will find them).

Should the NSS announcement be sent immediately, with a vague text - or -
should the NSS version annoucement be delayed until some later date?
(If yes, please suggest the date.)

Comment 28

[Kai Engert [:KaiE:]]

Will we require separate branch approval+ for landing the NSS update into mozilla-beta and mozilla-esr45, or are the existing tracking+ flags in this bugs sufficient for branch landing?

Comment 29

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8788866 [details] [diff] [review]
backport to NSS 3.21, v1

Review of attachment 8788866 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure what exactly is misleading here. But making this warning non-fatal for the branch makes sense.

Comment 30

[Kai Engert [:KaiE:]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #29)
> I'm not sure what exactly is misleading here.

For the lines that trigger the warning we have 

  if ()
    action();

However, those two lines start with a different mixed amount of tabs and spaces. The compiler apparently wants to warn that the indenting might mislead a human reader.

Comment 31

[Daniel Veditz [:dveditz]]

(In reply to Kai Engert (:kaie) from comment #27)
> I'd like to land the code into Firefox beta and ESR 45 as soon as possible
> (soon after the mit-september releases are done), and this will require me
> to make the new NSS releases publicly available on FTP/S3 (which means
> people will find them).

Are you landing "final" versions on the "beta" branch, and thus triggering what looks like an NSS "release"? Why so early in the cycle? If we need testing could we land these as "beta" versions of NSS and then change the definition (header value) to final later in the cycle?

How has this not come up before? We've fixed NSS bugs in the past and not released advisories for them until that version of Firefox was released.

> Should the NSS announcement be sent immediately, with a vague text - or -

What else is being fixed in these versions?

(In reply to Kai Engert (:kaie) from comment #28)
> Will we require separate branch approval+ for landing the NSS update into
> mozilla-beta and mozilla-esr45, or are the existing tracking+ flags in this
> bugs sufficient for branch landing?

The tracking flags mean tracking, nothing more. When a patch for a tracked bug is produced then the approval process is there to give release managers a chance to make sure the actual patch isn't too complicated/risky/untested/late for a given milestone.

Comment 32

[Kai Engert [:KaiE:]]

(In reply to Daniel Veditz [:dveditz] from comment #31)
> 
> Are you landing "final" versions on the "beta" branch, and thus triggering
> what looks like an NSS "release"?

Correct.

> Why so early in the cycle?

I just assumed you want all changes to mozilla-beta happening as early as possible.


> If we need
> testing could we land these as "beta" versions of NSS and then change the
> definition (header value) to final later in the cycle?

We could. What's the deadline for landing the final header version changes (and configure update to require the new version number) into the mozilla-beta and mozilla-esr45 branches?

We should start by testing with mozilla-central immediately, by uplifting a new NSS 3.27 beta to m-i today, I'll work to get this done. (It might have been a good idea to do this immediately after this fix landed into NSS trunk.)


> How has this not come up before? We've fixed NSS bugs in the past and not
> released advisories for them until that version of Firefox was released.

Not sure I understand the question. Yes, situations like that had been come up before. I made my comment to ensure everyone is aware of these needs for timing the announcements.

> 
> > Should the NSS announcement be sent immediately, with a vague text - or -
> 
> What else is being fixed in these versions?

Nothing else. The proposal is to produce security NSS releases, which are based on the currently used NSS branch releases used by the respective Firefox branches, plus this fix.

(These days we have one NSS branch for each Firefox branch.)


> The tracking flags mean tracking, nothing more.

Thanks for the clarification. We'll make attachments for the approval process.

Comment 33

[Kai Engert [:KaiE:]]

Regarding Dan's testing question:

- the fix from this bug hasn't produced any issues with the NSS own test suite

- a try build with this fix has now been started here:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a08925056fc

  (if try build succeeds, I'll uplift into mozilla-inbound by tomorrow)

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

It's too early to land this in mozilla-beta since the release date for 49 is now Sept. 20th. We should do this after we ship 49. Kai, I should have let you know, sorry about that.

Comment 35

[Kai Engert [:KaiE:]]

No problem, thanks for the update.

I'll work on it later next week.

Comment 36

[Kai Engert [:KaiE:]]

Comment on attachment 8788866 [details] [diff] [review]
backport to NSS 3.21, v1

Franziskus, the patch for NSS 3.21.x doesn't work with older gcc versions, apparently not on OSX. We need to limit -Wno-error=misleading-indentation to compiler versions that support it.

Comment 37

[Kai Engert [:KaiE:]]

Attachment: incremental for nss 3.21, restrict parameter -Wno-error=misleading-indentation to gcc >= 6

Based on other compiler version checks in the same file, I think this should work?

Comment 38

[Kai Engert [:KaiE:]]

Comment on attachment 8794281 [details] [diff] [review]
incremental for nss 3.21, restrict parameter -Wno-error=misleading-indentation to gcc >= 6

>diff --git a/coreconf/Werror.mk b/coreconf/Werror.mk
>+    CC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion))
>+    ifeq (,$(filter 0 1 2 3 4 5,$(word 1,$(CC_VERSION))))
>+      WARNING_CFLAGS += -Werror -Wno-error=misleading-indentation
>+    endif

We should also add a comment that explains the intention:

# Only add -Wno-error=misleading-indentation for gcc 6 and newer

Comment 39

[Kai Engert [:KaiE:]]

Comment on attachment 8794281 [details] [diff] [review]
incremental for nss 3.21, restrict parameter -Wno-error=misleading-indentation to gcc >= 6

sorry, let me redo this patch

Comment 40

[Kai Engert [:KaiE:]]

Attachment: incremental for nss 3.21, restrict parameter -Wno-error=misleading-indentation to gcc >= 6 (patch v2)

Comment 41

[Wan-Teh Chang]

Comment on attachment 8794281 [details] [diff] [review]
incremental for nss 3.21, restrict parameter -Wno-error=misleading-indentation to gcc >= 6

Review of attachment 8794281 [details] [diff] [review]:
-----------------------------------------------------------------

::: coreconf/Werror.mk
@@ +61,5 @@
>  
>    ifeq ($(NSS_ENABLE_WERROR),1)
> +    CC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion))
> +    ifeq (,$(filter 0 1 2 3 4 5,$(word 1,$(CC_VERSION))))
> +      WARNING_CFLAGS += -Werror -Wno-error=misleading-indentation

This patch is wrong. -Werror is fine for all versions of GCC and clang.

I suggest we simply remove -Wno-error=misleading-indentation. If an
NSS 3.21.x package needs to be built with the latest version of GCC,
the NSS 3.21.x package spec file should add -Wno-error=misleading-indentation.

We should not add such a change to an NSS branch without adding it to
the NSS trunk first.

Comment 42

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #41)
> 
> This patch is wrong. -Werror is fine for all versions of GCC and clang.

That's why I made the second version of the patch, which keeps -Werror in general.


> I suggest we simply remove -Wno-error=misleading-indentation. If an
> NSS 3.21.x package needs to be built with the latest version of GCC,
> the NSS 3.21.x package spec file should add
> -Wno-error=misleading-indentation.
> 
> We should not add such a change to an NSS branch without adding it to
> the NSS trunk first.

I don't intend to add this parameter to the NSS trunk. We don't need it there. We have reformatted our code on trunk, that prevents the error, and doesn't require the parameter.

This parameter is specifically suggested for the old NSS branch, where we shouldn't mess with cleaning up the bad whitespace.

Adding the patch to NSS directly is preferred, because this isn't just about creating NSS packages, but our continous integration includes systems that use gcc 6.

Comment 43

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #42)
> 
> Adding the patch to NSS directly is preferred, because this isn't just about
> creating NSS packages, but our continous integration includes systems that
> use gcc 6.

... and I'd like to build the NSS 3.21 release candidate on our continous integration infrastructure, just for completeness, to ensure all tests still pass on all platforms with this fix on that branch.

Comment 44

[Wan-Teh Chang]

GCC documentation says:

    In the case of mixed tabs and spaces, the warning uses
    the -ftabstop= option to determine if the statements
    line up (defaulting to 8).

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

NSS uses a tabstop of 8. So if GCC cannot handle that, we
should submit a bug report to GCC.

Comment 45

[Kai Engert [:KaiE:]]

Checked in to the NSS_3_26_BRANCH:
https://hg.mozilla.org/projects/nss/rev/d38536fcc726

NSS tests look good.

Checked in to the NSS_3_21_BRANCH:
https://hg.mozilla.org/projects/nss/rev/6883f1fc9129
https://hg.mozilla.org/projects/nss/rev/6ca604746182

NSS tests still running.

I'll shortly proceed with creating the NSS 3.21.2 and NSS 3.26.1 releases,

and will attach branch uplift patches for beta/esr approval by monday.

Comment 46

[Wan-Teh Chang]

Attachment: incremental for nss 3.21, fix misleading indentation

I compiled NSS_3_21_BRANCH with gcc 6.2.0. The misleading
indentations are not hard to fix. I think we should fix
the misleading indentations rather than disabling the
-Wmisleading-indentation compiler warning.

Comment 47

[Wan-Teh Chang]

Attachment: incremental for nss 3.21, fix misleading indentation (patch v2)

I updated my patch because Kai's patch has been checked in.

I hope the benefits of compiling NSS_3_21_BRANCH with -Wmisleading-indentation
are clear.

Comment 48

[Kai Engert [:KaiE:]]

Wan-Teh, I appreciate your intention to simplify, but let me say:
- I'm trying to minimize patches that backport security fixes as much as possible,
  and white space changes in particular can usually create confusion very easily,
  and distract from the real change
- I had not attempted to go the whitespace fixing approach in the first place,
  because my expectation was that a much larger number of places might have
  required fixing, so it's good that only very few places would need a fix.
- the benefit of the compiler warning seems negligible in this particular scenario,
  because we know the code is correct, and we're not developing new code on this branch.

My preference is to keep things simple, and because buildbot seems to currently build fine with the code as checked in, avoiding additional commits seems simpler to me, so in my opinion your patch isn't necessary.

But if Franziskus thinks differently and prefers the whitespace cleanup and gives you r+, I'll check it in, and will have to trigger another round of test builds afterwards.

Comment 49

[Wan-Teh Chang]

I was mainly curious about the speculation that -Wmisleading-indentation
cannot handle mixed tabs and spaces correctly. I found that GCC handles
mixed tabs and spaces correctly in all the four files it warns about.
* The two files in lib/dbm/src/ use a tab stop of 4. The warnings could
  also be fixed by compiling those files with the -ftabstop=4 option.
* The other two files simply have incorrect indentations.
I'm glad I don't need to submit a bug report to GCC.

I also wanted to know if any of the incorrect indentations is a bug.
Fortunately none of them is. Without looking at the warnings, we cannot
be sure.

So, whether you accept my patch or not is secondary to me.

Comment 50

[Kai Engert [:KaiE:]]

Comment on attachment 8794355 [details] [diff] [review]
incremental for nss 3.21, fix misleading indentation (patch v2)

Wan-Teh, thanks a lot for your thorough verifications!
I'll defer to Franziskus if we should check this in, or can skip it.

Comment 51

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8794355 [details] [diff] [review]
incremental for nss 3.21, fix misleading indentation (patch v2)

Review of attachment 8794355 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for the investigation Wan-Teh.
But since there's no bug I think we should leave the 3.21 branch alone.

Comment 52

[Kai Engert [:KaiE:]]

Attachment: Uplift NSS 3.26.1 to the Firefox-Beta-50 branch

Firefox-50 beta branch currently uses NSS 3.26.
A version NSS 3.26.1 has been released, which contains this bugfix as the only change since NSS 3.26.

This patch uplifts to NSS 3.26.1, which includes this bugfix, plus version number changes, only.

Comment 53

[Kai Engert [:KaiE:]]

Attachment: Uplift NSS 3.21.2 to the Firefox-ESR-45 branch

Firefox-ESR-45 branch currently uses NSS 3.21.1
A version NSS 3.21.2 has been released, which contains this bugfix as the only change since NSS 3.21.1

This patch uplifts to NSS 3.21.2, which includes this bugfix, plus version number changes, only.

Comment 54

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8795159 [details] [diff] [review]
Uplift NSS 3.26.1 to the Firefox-Beta-50 branch

NSS update, Beta50+

Comment 55

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8795164 [details] [diff] [review]
Uplift NSS 3.21.2 to the Firefox-ESR-45 branch

NSS update, ESR45.5+

Comment 56

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Dan, based on comment 12, it seems Fx51 will be on NSS 3.27. What should we do about status-51 "affected" on this bug?

Comment 57

[Kai Engert [:KaiE:]]

(In reply to Ritu Kothari (:ritu) from comment #56)
> Hi Dan, based on comment 12, it seems Fx51 will be on NSS 3.27. What should
> we do about status-51 "affected" on this bug?

Firefox 51 is already using NSS 3.27, which already contains the fix for this bug.
I'm flipping the bit to fixed.

Comment 58

[Kai Engert [:KaiE:]]

mozilla-beta tree is closed for "uplift week".
Can you please advise when I should check in?

Comment 59

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/3ec640d349ad

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d42771a06d13

Comment 61

[Huzaifa Sidhpurwala]

Hi,

I am trying to understand if this is really a security flaw? I understand that barrett reduction is much better, but is the div_spoiler implementation broken, ie it did not implement the fix correctly?

If not, that this is just security hardening and not a flaw imo

Comment 62

[Franziskus Kiefer [:franziskus]]

(In reply to Huzaifa Sidhpurwala from comment #61)
> Hi,
> 
> I am trying to understand if this is really a security flaw? I understand
> that barrett reduction is much better, but is the div_spoiler implementation
> broken, ie it did not implement the fix correctly?
> 
> If not, that this is just security hardening and not a flaw imo

Yes this is a security flaw. See links comment 1, i.e. division on Intel CPUs is not constant time, which provides the attacker with a padding oracle.

Comment 63

[Huzaifa Sidhpurwala]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #62)
> (In reply to Huzaifa Sidhpurwala from comment #61)
> > Hi,
> > 
> > I am trying to understand if this is really a security flaw? I understand
> > that barrett reduction is much better, but is the div_spoiler implementation
> > broken, ie it did not implement the fix correctly?
> > 
> > If not, that this is just security hardening and not a flaw imo
> 
> Yes this is a security flaw. See links comment 1, i.e. division on Intel
> CPUs is not constant time, which provides the attacker with a padding oracle.

I know, but the workaround to that is to use the divspoiler mechanism, which was used everywhere, barrett reduction is just the better way of doing it.

Comment 64

[Huzaifa Sidhpurwala]

Note:

Just to clarify the above, i dont understand why a CVE id was assigned in the first place. barrett reduction is a much better way of doing constant-time operation, but div_spoiler is not vulnerable in the first place (not perfect but not vulnerable).

So even that the older method isnt really vulnerable, this is not really a security issue.

Comment 65

[Huzaifa Sidhpurwala]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #62)
> (In reply to Huzaifa Sidhpurwala from comment #61)
> > Hi,
> > 
> > I am trying to understand if this is really a security flaw? I understand
> > that barrett reduction is much better, but is the div_spoiler implementation
> > broken, ie it did not implement the fix correctly?
> > 
> > If not, that this is just security hardening and not a flaw imo
> 
> Yes this is a security flaw. See links comment 1, i.e. division on Intel
> CPUs is not constant time, which provides the attacker with a padding oracle.

No, this is not a security flaw. div_spoiler solves the problem, its not perfect but works, barrett reduction is just a better way of doing things.

Comment 66

[Kai Engert [:KaiE:]]

Huzaifa, I'd like to suggest, please clarify what action you expect.

Comment 67

[Huzaifa Sidhpurwala]

(In reply to Kai Engert (:kaie) from comment #66)
> Huzaifa, I'd like to suggest, please clarify what action you expect.

Not sure if this makes any difference now. I want to suggest we reject this CVE id. This is not a security flaw. div_spoiler works well, barrett reduction is just a better method of handling things. This is an enhancement bug not a security flaw.

Comment 68

[Franziskus Kiefer [:franziskus]]

Huzaifa: I think you're missing the point. At the danger of repeating myself let me explain again what this bug is about.
The original code added the so called divSpoiler to |macStart - scanStart| in the hope that this would make CPUs compute |divq| operations in constant time. That's however not the case (at least not everywhere).
The new code, using barrett reduction, aims to bypass the problem of non-constant-time div operations on CPUs by not using it. Barrett instead only subtracts.
So this change has nothing to with enhancement but is fixing a security issue. Because observing timing differences in CPU instructions is relatively hard, this is sec-moderate.

Comment 69

[Huzaifa Sidhpurwala]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #68)
> Huzaifa: I think you're missing the point. At the danger of repeating myself
> let me explain again what this bug is about.
> The original code added the so called divSpoiler to |macStart - scanStart|
> in the hope that this would make CPUs compute |divq| operations in constant
> time. That's however not the case (at least not everywhere).
> The new code, using barrett reduction, aims to bypass the problem of
> non-constant-time div operations on CPUs by not using it. Barrett instead
> only subtracts.
> So this change has nothing to with enhancement but is fixing a security
> issue. Because observing timing differences in CPU instructions is
> relatively hard, this is sec-moderate.

Ok that makes it clear, thanks!