openssh bugs - Mar 2012 - [Bug 1991] New: openssl version checking needs updating

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

             Bug #: 1991
           Summary: openssl version checking needs updating
    Classification: Unclassified
           Product: Portable OpenSSH
           Version: 5.9p1
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Miscellaneous
        AssignedTo: unassigned-bugs at mindrot.org
        ReportedBy: vapier at gentoo.org


Created attachment 2137
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2137
update openssl ver check

with openssl-1.0.0, they've started a new binary compatibility scheme. 
in the past, only patchset versions were compatible (so
0.9.8[abcdefgh...]).  but now, minor versions are compatible as well. 
so 1.0.[01234...] should be acceptable.

as such, the seed_rng() check in entropy.c needs updating.  perhaps
something like the (compile-only tested) attached patch.

--- a/entropy.c
+++ b/entropy.c
@@ -211,9 +211,14 @@ seed_rng(void)
 #endif
    /*
     * OpenSSL version numbers: MNNFFPPS: major minor fix patch status
-    * We match major, minor, fix and status (not patch)
+    * We match major, minor, fix and status (not patch) for <1.0.0.
+    * After that, we acceptable compatible minor versions (so we
+    * allow 1.0.1 to work with 1.0.0).
     */
-   if ((SSLeay() ^ OPENSSL_VERSION_NUMBER) & ~0xff0L)
+   u_long bldver = OPENSSL_VERSION_NUMBER & ~0xff0L;
+   u_long runver = SSLeay() & ~0xff0L;
+   if ((bldver >> 12) < 0x10000 && bldver != runver) ||
+       (bldver >> 12) >= 0x10000 && (runver >> 12) <
(bldver >> 12)))
        fatal("OpenSSL version mismatch. Built against %lx, you "
            "have %lx", (u_long)OPENSSL_VERSION_NUMBER, SSLeay());

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

--- Comment #1 from Darren Tucker <dtucker at zip.com.au> 2012-03-30
10:45:56 EST ---
Comment on attachment 2137
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2137
update openssl ver check
>+		(bldver >> 12) >= 0x10000 && (runver >> 12) <
(bldver >> 12)))
This is going to drop the status nybble off when it's shifted, which
means you can build against a dev version of openssl and run against a
release one.  There's no guarantee that's going to be binary
compatible, though.  The current check will catch that case, though.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> 2012-03-30
11:05:17 EST ---
Comment on attachment 2137
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2137
update openssl ver check

Also:
>+	 * After that, we acceptable compatible minor versions 
you're accepting compatible *fix* versions.
>+		(bldver >> 12) >= 0x10000 && (runver >> 12) <
(bldver >> 12)))
that's not going to stop newer major or minor library versions from
being built against old headers.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au

--- Comment #3 from Darren Tucker <dtucker at zip.com.au> 2012-03-30
11:17:31 EST ---
I think it would be simpler and more correct to keep the existing logic
and just change the mask size, eg:

u_long version_mask = SSLeay() >= 0x10000000 ?  ~0xffff0L : ~0xff0L;
if ((SSLeay() ^ OPENSSL_VERSION_NUMBER) & version_mask)
        fatal("OpenSSL version mismatch. Built against %lx, you "
            "have %lx", (u_long)OPENSSL_VERSION_NUMBER, SSLeay());

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2137|0                           |1
        is obsolete|                            |
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned-bugs at mindrot.org |djm at mindrot.org
   Attachment #2139|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #4 from Damien Miller <djm at mindrot.org> 2012-03-30 11:18:19
EST ---
Created attachment 2139
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2139
Improved test

This check is a little more strict:

It matches the patch version (so 1.2.0 will not work with an OpenSSH
built against 1.0.1). This is a bit more paranoid than the original
patch, but looser than what we have at present.

It also checks disables the laxity if the build or runtime versions are
not official releases. My rationale here is that binary compatibility
might be broken in unreleased versions.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2140|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #5 from Damien Miller <djm at mindrot.org> 2012-03-30 11:28:27
EST ---
Created attachment 2140
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2140
Improved improved test

Darren's right, as usual.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2140|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1930
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #6 from Damien Miller <djm at mindrot.org> 2012-03-30 11:35:28
EST ---
"improved improved test" patch applied and will be in openssh-6.0 (due
very soon)

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

--- Comment #7 from Darren Tucker <dtucker at zip.com.au> 2012-03-30
11:50:11 EST ---
Thinking about it some more, the cases you need to consider:
#1: you upgrade openssl to a newer fix version.  obviously you don't
want ssh to stop working and with this diff, it won't.

#2: you upgrade ssh with something built against the same major and
minor version but a newer fix version.  Right now, you can't deploy
that unless you upgrade openssl first.

is #2 a reasonable thing to do?  I would argue that it is.

Damien's counter-argument is from the OpenSSL home page: "OpenSSL 1.0.1
is now available, including new features".

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

Tim Rice <tim at multitalents.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tim at multitalents.net

--- Comment #8 from Tim Rice <tim at multitalents.net> 2012-03-30 14:18:55
EST ---
(In reply to comment #7)
> Thinking about it some more, the cases you need to consider:
> #1: you upgrade openssl to a newer fix version.  obviously you don't
> want ssh to stop working and with this diff, it won't.
> 
> #2: you upgrade ssh with something built against the same major and
> minor version but a newer fix version.  Right now, you can't deploy
> that unless you upgrade openssl first.
> 
> is #2 a reasonable thing to do?  I would argue that it is.
> 
> Damien's counter-argument is from the OpenSSL home page: "OpenSSL
1.0.1
> is now available, including new features".
#2 would allow "bad" practice in the general sense.
Meaning, while it may be reasonable for a binary built against an older
lib to be expected to run with a newer lib, it is not reasonable to
expect a binary built with a newer lib to run with an older lib.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

Tomas Mraz <t8m at centrum.cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |t8m at centrum.cz

--- Comment #9 from Tomas Mraz <t8m at centrum.cz> 2012-03-30 17:39:19 EST
---
Note that beta versions on the same fix release (1.0.z should be ABI
compatible. Only when the major or minor release changes there should
be ABI breakers (that is when x or y in x.y.z changes).

Also as the patch level (the letter after version) changes there should
be strictly only bugfixes, these should be even forward-backwards
compatible.

So for the after 1.0 versions I'd suggest the version_mask to be
~0xfffffL

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1991

--- Comment #10 from Darren Tucker <dtucker at zip.com.au> 2012-03-30
19:09:45 EST ---
(In reply to comment #9)
> Note that beta versions on the same fix release (1.0.z should be ABI
> compatible. Only when the major or minor release changes there should
> be ABI breakers (that is when x or y in x.y.z changes).
You'd hope so, however from the CHANGES file in openssl 1.0.1 under
"Changes between 1.0.0h and 1.0.1" (a "fix" release, in
openssl's
parlance) shows, amongst other things:

  *) Functions FIPS_mode_set() and FIPS_mode() which call the
underlying
     FIPS modules versions.
     [Steve Henson]

  *) [...] This enables the following EC_METHODs:
         EC_GFp_nistp224_method()
         EC_GFp_nistp256_method()
         EC_GFp_nistp521_method()

so, new functions introduced in "fix" releases.  Given this, we are
yet
to be convinced that "fix" releases both are forward and backward ABI
compatible.
> Also as the patch level (the letter after version) changes there should
> be strictly only bugfixes, these should be even forward-backwards
> compatible.
Patch level is covered by the 0xff0 mask in both cases.
> So for the after 1.0 versions I'd suggest the version_mask to be
> ~0xfffffL
That'd allow development and release versions to mix too.  For now
we're only considering release versions.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.