Bug#802671: Tentative patches for version 1.44

[Raphael Hertzog]

On Fri, 04 Dec 2015, Markus Koschany wrote:
> thanks for your work on this bug. We intend to upload version 1.51 of
> bouncycastle to unstable this weekend since we were able to upgrade all
> reverse-dependencies except one so far. Are there any new information
> regarding the patches for Jessie? Shall we still wait with an upload or
> is it safe to use the three existing patches?

Upstream told me that the supplementary fixes will be released in
version 1.54 as they are not urgent and that we can release the current
set of fixes for now.

He suggested to run the ECPointTest unit tests against the backported code
though. I did not check yet what this involves... and whether unit tests
are run automatically during build or not.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/

__
This is the maintainer address of Debian's Java team
. 
Please use
debian-j...@lists.debian.org for discussions and questions.

Bug#802671: Tentative patches for version 1.44

[Raphael Hertzog]

Hi,

On Fri, 04 Dec 2015, Markus Koschany wrote:
> thanks for your work on this bug. We intend to upload version 1.51 of
> bouncycastle to unstable this weekend since we were able to upgrade all
> reverse-dependencies except one so far. Are there any new information
> regarding the patches for Jessie? Shall we still wait with an upload or
> is it safe to use the three existing patches?

I pinged Peter since I had no further news from him. I propose to wait a
little bit longer for the jessie upload.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/

__
This is the maintainer address of Debian's Java team
. 
Please use
debian-j...@lists.debian.org for discussions and questions.

Bug#802671: Tentative patches for version 1.44

[Markus Koschany]

On Thu, 26 Nov 2015 13:58:14 +0100 Raphael Hertzog 
wrote:
> On Fri, 20 Nov 2015, Raphael Hertzog wrote:
> > On Fri, 23 Oct 2015, Raphael Hertzog wrote:
> > > I have asked an upstream developer (Peter Dettman) to review it.
> > 
> > He reviewed them and came up with further suggestions. So there's a third
> > patch (attached) to apply on top of the two patches that I already
> > submitted. I sent him the third patch for review too.
> 
> Tha patch is also OK according to Peter. However he asked me to not
> yet release the update as further improvements in public key/point
> validation are being made.
> 
> I'll the bug in the loop when I have more details, in the next few days in
> theory.

Hello Raphael,

thanks for your work on this bug. We intend to upload version 1.51 of
bouncycastle to unstable this weekend since we were able to upgrade all
reverse-dependencies except one so far. Are there any new information
regarding the patches for Jessie? Shall we still wait with an upload or
is it safe to use the three existing patches?

Regards,

Markus



signature.asc
Description: OpenPGP digital signature
__
This is the maintainer address of Debian's Java team
. 
Please use
debian-j...@lists.debian.org for discussions and questions.

Bug#802671: Tentative patches for version 1.44

[Raphael Hertzog]

On Fri, 20 Nov 2015, Raphael Hertzog wrote:
> On Fri, 23 Oct 2015, Raphael Hertzog wrote:
> > I have asked an upstream developer (Peter Dettman) to review it.
> 
> He reviewed them and came up with further suggestions. So there's a third
> patch (attached) to apply on top of the two patches that I already
> submitted. I sent him the third patch for review too.

Tha patch is also OK according to Peter. However he asked me to not
yet release the update as further improvements in public key/point
validation are being made.

I'll the bug in the loop when I have more details, in the next few days in
theory.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/

__
This is the maintainer address of Debian's Java team
. 
Please use
debian-j...@lists.debian.org for discussions and questions.

Bug#802671: Tentative patches for version 1.44

[Raphael Hertzog]

On Fri, 23 Oct 2015, Raphael Hertzog wrote:
> I have asked an upstream developer (Peter Dettman) to review it.

He reviewed them and came up with further suggestions. So there's a third
patch (attached) to apply on top of the two patches that I already
submitted. I sent him the third patch for review too.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/
Implement further updates suggested by Petter Dettman after review
of the first two patches. His intructions were the following:

> I think the treatment of the cofactor (h, getH()) for
> ECCurve.Fp needs more attention. The current validity checks for ECPoint
> rely on there being a cofactor provided to check against, but as updated
> by this patch, all ECCurve.Fp simply return null from getH().
> 
> Specifying the cofactor for all the "built-in" curves was preparatory
> work that these validation commits relied on so in their current state
> the patches effectively skip an important check for most of the built-in
> Fp curves, which probably defeats the purpose.
> 
> The "h == null" in ECPoint.satisfiesCofactor is not ideal even in the
> current code, but it's tolerable if all the built-in curves actually do
> specify a cofactor.
> 
> I would recommend that you add the ECCurve.Fp constructor that allows to
> specify cofactor (and order if you like), then change all the curve
> registry classes:
> ECGOST3410NamedCurves
> SECNamedCurves
> TeleTrusTNamedCurves
> X962NamedCurves
> 
> so that they use the new constructor. Then change ECCurve.java so that
> the cofactor (and order - can keep calling them h, n in the code) are
> actually stored in the base class and returned correctly for ECCurve.Fp.
> 
> All the values you need are of course available in the latest code.
> Unfortunately there's quite a lot of them, but the changes should be
> fairly mechanical.

--- a/src/org/bouncycastle/asn1/cryptopro/ECGOST3410NamedCurves.java
+++ b/src/org/bouncycastle/asn1/cryptopro/ECGOST3410NamedCurves.java
@@ -6,6 +6,7 @@ import java.util.Hashtable;
 
 import org.bouncycastle.asn1.DERObjectIdentifier;
 import org.bouncycastle.crypto.params.ECDomainParameters;
+import org.bouncycastle.math.ec.ECConstants;
 import org.bouncycastle.math.ec.ECCurve;
 import org.bouncycastle.math.ec.ECFieldElement;
 import org.bouncycastle.math.ec.ECPoint;
@@ -27,7 +28,9 @@ public class ECGOST3410NamedCurves
 ECCurve.Fp curve = new ECCurve.Fp(
 mod_p, // p
 new BigInteger("115792089237316195423570985008687907853269984665640564039457584007913129639316"), // a
-new BigInteger("166")); // b
+new BigInteger("166"), // b
+mod_q,
+ECConstants.ONE);
 
 ECDomainParameters ecParams = new ECDomainParameters(
 curve,
@@ -44,7 +47,9 @@ public class ECGOST3410NamedCurves
 curve = new ECCurve.Fp(
 mod_p, // p
 new BigInteger("115792089237316195423570985008687907853269984665640564039457584007913129639316"),
-new BigInteger("166"));
+new BigInteger("166"),
+mod_q,
+ECConstants.ONE);
 
 ecParams = new ECDomainParameters(
 curve,
@@ -61,7 +66,9 @@ public class ECGOST3410NamedCurves
 curve = new ECCurve.Fp(
 mod_p, // p
 new BigInteger("57896044618658097711785492504343953926634992332820282019728792003956564823190"), // a
-new BigInteger("28091019353058090096996979000309560759124368558014865957655842872397301267595")); // b
+new BigInteger("28091019353058090096996979000309560759124368558014865957655842872397301267595"), // b
+mod_q,
+ECConstants.ONE);
 
 ecParams = new ECDomainParameters(
 curve,
@@ -78,7 +85,9 @@ public class ECGOST3410NamedCurves
 curve = new ECCurve.Fp(
 mod_p, // p
 new BigInteger("70390085352083305199547718019018437841079516630045180471284346843705633502616"),
-new BigInteger("32858"));
+new BigInteger("32858"),
+mod_q,
+ECConstants.ONE);
 
 ecParams = new ECDomainParameters(
 curve,
@@ -94,7 +103,9 @@ public class ECGOST3410NamedCurves
 curve = new ECCurve.Fp(
 mod_p, // p
 new BigInteger("70390085352083305199547718019018437841079516630045180471284346843705633502616"), // a
-new BigInteger("32858")); // b
+new BigInteger("32858"), // b
+mod_q,
+ECConstants.ONE);
 
 ecParams = new ECDomainParameters(
 curve,
--- a/src/org/bouncycastle/asn1/sec/SECNamedCurves.java
+++ b/src/org/bouncycastle/asn1/sec/SECNamedCurves.java
@@ -36,7 +36,7 @@ public class SECNamedCurves
 BigInteger n = 

Bug#802671: Tentative patches for version 1.44

[Raphael Hertzog]

Hello,

I have backported the relevant commits to version 1.44 and the result
is in the attached patches. The package builds fine but I have not
tested it and I'm not sure how to properly test it... if you have
suggestions, I'm happy to hear them.

I have asked an upstream developer (Peter Dettman) to review it.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/
>From 5cb2f0578e6ec8f0d67e59d05d8c4704d8e05f83 Mon Sep 17 00:00:00 2001
From: Peter Dettman 
Date: Tue, 22 Jul 2014 19:23:34 +0700
Subject: [PATCH] Add automatic EC point validation for decoded points and for
 multiplier outputs.
Origin: upstream, https://github.com/bcgit/bc-java/commit/5cb2f05
Bug-Debian: https://bugs.debian.org/802671

Backporting notes of Raphaël Hertzog:
* core/src/main/java/org/bouncycastle/ in current git
  was src/org/bouncycastle/ in 1.44
* DSTU4145PointEncoder.java does not exist in 1.44 (introduced
  in 158b54f). Dropped the changes.
* AbstractECMultiplier.java does not exist in 1.44 but changes to
  AbstractECMultiplier.java mean that we must run
  ECAlgorithms.validatePoint() on any result of the multiply() function of
  any object implementing ECMultiplier. Done on:
  - FpNafMultiplier.java
  - ReferenceMultiplier.java
  - WNafMultiplier.java
  - WTauNafMultiplier.java
* …/math/ec/custom/* were not present in 1.44. Dropped the corresponding
  changes.
* Remaining changes have been manually backported:
  - ECPointTest.java: done
  - ReferenceMultiplier.java: done, added validatePoint() call on result
  - ECAlgorithms.java: done
  - ECPoint.java: done
- Fp does not yet support getCompressionYTilde(), dropped from
  AbstractFp
- F2m does not yet support checkCurveEquation()
- dropped constructors accepting 4 params (with "zs") as ECPoint()
  does not support it, and dropped all code path that made use of this.zs
  since it's not available, basically everything related to non-affine
  coordinate system
  - ECCurve.java:
- Hunk 1: validatePoint() not backported as there is no createPoint() call
  to replace. Instead ensure decodePoint() return value satisfies
  ECAlgorithms.validatePoint()
- Hunk 2: no importPoint() (and no createPoint() usage found)
- Hunk 3: useless (no-op change)
- Hunk 4: useless (no-op change)
- Hunk 5: validation on generated point at end of function
- Hunk 6: done
- Hunk 7: done (auto-applied)
- Hunk 8/9: ECCurve is abstract and has no constructor, don't call
  parent constructors in Fp constructors (which happens in code
  from hunk 7 adding AbstractFp)
- Hunk 10: ECCurve.Fp does not have decompressPoint() in 1.44, so the whole
  AbstractFp class was in fact useless, drop it and make Fp extends
  ECCurve again.
  End of hunk not applied, the AbstractF2m class is not needed as its
  sole purpose is to factorize a call to buildField() that version
  1.44 does not have.
- Hunk 11/12/13: Not applied as we don't introduce AbstractF2m.
- Hunk 14: yp is already initialized as null in 1.44.
- Hunk 15: decompressPoint() is really implemented differently... and
  even has different parameters. Just add the final check for yp==null
  and don't change the logic in the function.
---
 .../bouncycastle/asn1/ua/DSTU4145PointEncoder.java |  20 +-
 .../bouncycastle/math/ec/AbstractECMultiplier.java |   8 +-
 .../org/bouncycastle/math/ec/ECAlgorithms.java |  56 -
 .../java/org/bouncycastle/math/ec/ECCurve.java | 183 +-
 .../java/org/bouncycastle/math/ec/ECPoint.java | 270 ++---
 .../bouncycastle/math/ec/ReferenceMultiplier.java  |  28 +--
 .../math/ec/custom/djb/Curve25519.java |  29 +--
 .../math/ec/custom/djb/Curve25519Point.java|  17 +-
 .../math/ec/custom/sec/SecP192K1Curve.java |  33 +--
 .../math/ec/custom/sec/SecP192K1Point.java |  19 +-
 .../math/ec/custom/sec/SecP192R1Curve.java |  29 +--
 .../math/ec/custom/sec/SecP192R1Point.java |  19 +-
 .../math/ec/custom/sec/SecP224K1Curve.java |  33 +--
 .../math/ec/custom/sec/SecP224K1Point.java |  19 +-
 .../math/ec/custom/sec/SecP224R1Curve.java |  29 +--
 .../math/ec/custom/sec/SecP224R1Point.java |  18 +-
 .../math/ec/custom/sec/SecP256K1Curve.java |  33 +--
 .../math/ec/custom/sec/SecP256K1Point.java |  19 +-
 .../math/ec/custom/sec/SecP256R1Curve.java |  29 +--
 .../math/ec/custom/sec/SecP256R1Point.java |  18 +-
 .../math/ec/custom/sec/SecP384R1Curve.java |  29 +--
 .../math/ec/custom/sec/SecP384R1Point.java |  18 +-
 .../math/ec/custom/sec/SecP521R1Curve.java |  29 +--
 .../math/ec/custom/sec/SecP521R1Point.java |  18 +-
 .../org/bouncycastle/math/ec/test/ECPointTest.java |  33 +--
 

Bug#802671: Tentative patches for version 1.44

[Emmanuel Bourg]

Thank you for the backport Raphael, I started working on the backport
for 1.49 and I've interpreted similarly the changes required. The patch
could be slightly simplified by moving the satisfiesCurveEquation()
functions directly into the two ECPoint subclasses instead of
introducing an intermediary abstract class.

As for testing I face the issue, some help from upstream developers
would be welcome.

Emmanuel Bourg

__
This is the maintainer address of Debian's Java team
. 
Please use
debian-j...@lists.debian.org for discussions and questions.