Bug 229029 - net/rdesktop Fails to build with OpenSSL 1.1
Summary: net/rdesktop Fails to build with OpenSSL 1.1
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Kurt Jaeger
URL: https://reviews.freebsd.org/D18566
Keywords:
: 235600 (view as bug list)
Depends on:
Blocks: 228865
  Show dependency treegraph
 
Reported: 2018-06-14 20:18 UTC by Bernard Spil
Modified: 2019-02-22 17:59 UTC (History)
9 users (show)

See Also:
bugzilla: maintainer-feedback? (gregf)


Attachments
openssl.patch (1.77 KB, patch)
2018-06-26 03:19 UTC, Greg Fitzgerald
no flags Details | Diff
openssl2.patch (3.52 KB, patch)
2018-06-26 03:19 UTC, Greg Fitzgerald
no flags Details | Diff
rdesktop.diff (7.08 KB, patch)
2018-08-20 14:45 UTC, Greg Fitzgerald
no flags Details | Diff
Upgrade rdesktop to 1.8.4 (1.27 KB, text/plain)
2019-01-18 22:59 UTC, Jack
no flags Details
Log of rdesktop 1.8.4 building with openssl 1.1.1a on FreeBSD 12-STABLE (47.07 KB, text/plain)
2019-01-18 23:05 UTC, Jack
no flags Details
rdesktop-1.8.4.diff (3.46 KB, patch)
2019-01-23 13:09 UTC, Greg Fitzgerald
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bernard Spil freebsd_committer 2018-06-14 20:18:58 UTC
> ssl.c:91:9: error: variable has incomplete type 'BIGNUM' (aka 'struct bignum_st')

During BSDCan 2018 the intention to update OpenSSL in base to 1.1.x branch was documented.

Intention is to update 12-STABLE to current 1.1.0 and subsequently update it to 1.1.1 when that is released. The intent to update OpenSSL to 1.1 in 12 has now officially been documented in Bug #228912 

Poudriere log: https://keg.brnrd.eu/data/111amd64-default-openssl110/2018-06-11_10h42m37s/logs/errors/rdesktop-1.8.3.log
Comment 1 Greg Fitzgerald 2018-06-26 03:19:29 UTC
Created attachment 194639 [details]
openssl.patch
Comment 2 Greg Fitzgerald 2018-06-26 03:19:52 UTC
Created attachment 194640 [details]
openssl2.patch
Comment 3 Greg Fitzgerald 2018-06-26 03:21:07 UTC
I found Archlinux was applying both of these patches to fix the OpenSSL problem. I tried tossing them in the files directory but they're not applying right and I"m not sure why? If someone can help me fix this issue?
Comment 4 Bernard Spil freebsd_committer 2018-07-07 11:18:37 UTC
Hi Greg,

These are git diffs. You should be able to apply them manually from WRKSRC using  `patch -p1 < openssl.patch` etc. I tend to convert these using a sed one-liner, see https://wiki.freebsd.org/BernardSpil/PortingNotes 'Convert git diff'

Once applied to the source, please use `make makepatch` to generate the patches and use `svn diff` to generate a diff to upload here as a patch.
Comment 5 Greg Fitzgerald 2018-08-20 14:45:53 UTC
Created attachment 196387 [details]
rdesktop.diff
Comment 6 Kubilay Kocak freebsd_committer freebsd_triage 2018-08-21 01:49:38 UTC
Reporter is committer, assign accordingly.
Comment 7 Greg Fitzgerald 2018-10-14 21:17:49 UTC
Everyone busy or am I supposed to be doing something here, I see it needs maintainer feedback, but I'm not sure on what.
Comment 8 Conrad Meyer freebsd_committer 2018-11-08 21:47:41 UTC
I need this port :-).

I'm happy to test and/or commit, but I need approval from someone with a ports bit.
Comment 9 Conrad Meyer freebsd_committer 2018-11-09 18:30:42 UTC
(In reply to Greg Fitzgerald from comment #5)
Upstream has both of these patches in git; the first since September 2016, for what it's worth:

https://github.com/rdesktop/rdesktop/commit/50b39d114aa9d528c43e7aeefce37b3542f44ba5

Why don't we just update rdesktop to a newer git version instead of adding more patch files?  Upstream is active but hasn't issued a new "release" since 2015, which is just silly.  They're essentially a rolling release git project now.
Comment 10 Greg Fitzgerald 2018-11-15 21:59:04 UTC
(In reply to Conrad Meyer from comment #9)
I would be in favor of changing this to a git based port if everyone is OK with that. I just don't have the time to do it for a bit yet. If you are in a hury  Conrad to get that done I'll approve anything you get working. If not I'll work on it soon as I can.
Comment 11 Conrad Meyer freebsd_committer 2018-11-15 21:59:58 UTC
No rush for me anymore — vangyzen@ pointed me at Remmina as a suitable replacement.
Comment 12 Greg Fitzgerald 2018-12-02 14:19:25 UTC
Could someone please review this and get it pushed so rdesktop will work in 12? I'll get on uinsg a git snapshot soon as I have some extra time, but for now this should fix the build errors.
Comment 13 Conrad Meyer freebsd_committer 2018-12-02 17:39:09 UTC
Reviewing 2018-08-20 rdesktop.diff:

This patch raises some questions of its own, like, "why is rdesktop reimplementing RSA encryption instead of just using openssl?".  This would probably be easier (for me) in phabricator, FWIW.

-----

patch-openssl mostly looks like a straightforward translation of the existing code.

The line:

  algor = X509_PUBKEY_get0_param(NULL, NULL, 0, &algor, key);

Is slightly off — the 3rd parameter should just be NULL as well.  Fortunately, zero has the same value as null and is implicitly converted to 'int *'.  (Oh, and as the second patch points out, the return value is an int, not a pointer -- this code simply doesn't work.)

I have no idea if the part at 'X509_PUBKEY_set0_param' accomplishes the same goals as the prior code, but it looks reasonable.

The hunk at 201,14 +209,24 is bogus:

++	BIGNUM *e = NULL;
++	BIGNUM *n = NULL;
...
++	RSA_get0_key(rkey, &e, &n, NULL);

RSA_get0_key has the signature (r, *n, *e, *d).  We're grabbing the modulus as 'e' and the public exponent as 'n'.  Both are components of the public key, so it 's not like this is a private key leak, but the variables are now misnamed.

It then proceeds to copy those incorrect values to its own modulus/exponent out-parameters.  **This patch is totally broken.**

-----

patch-openssl2 corrects some deficiencies in the first one and looks fine.

-----

Port mechanics: looks fine to me (keeping in mind I don't have a ports bit).  Usual convention is to have a single patch against a given file (e.g., ssl.c) but I don't especially like that convention and it doesn't really make sense when incorporating a series of upstream patches, like here.

-----

While this would fix the build on openssl 1.1.0+, it introduces a regression in rdssl_rkey_get_exp_mod().  I don't think I would approve this as-is.
Comment 14 Greg Fitzgerald 2018-12-02 17:45:04 UTC
(In reply to Conrad Meyer from comment #13)
Conrad,

Thanks for the detailed review. I'm not qualified to handle this patch I fear. I don't know much of any c++ and I never worked with openssl outside of go and ruby. 

This patch is up streamed now so I'm not sure if we should just abandon rdesktop in ports because the code isn't safe? 

Either way I'm going to need some help on how to proceed from here.
Comment 15 Conrad Meyer freebsd_committer 2018-12-02 18:29:28 UTC
(In reply to Greg Fitzgerald from comment #14)
Unfortunately, the n/e swap was fixed in this upstream commit whose primary scope was unrelated and whose commit message doesn't even mention the correction:

https://github.com/rdesktop/rdesktop/commit/a3dfceefc2c729243b71270e3f503fa2dd57ec8d

It looks like it might also be significant for rdesktop functionality on OpenSSL 1.1.0+ anyway, so maybe we just take the whole patch?  Unfortunately again, it definitely won't apply cleanly because it modifies the same code as all of these intermediary patches that were committed after patch-openssl2 and before a3dfceef:

* https://github.com/rdesktop/rdesktop/commit/71bf45a5d7ac6dcd33324e9adc3df75daff2c59d#diff-5931449ee480a798cabbfe5a0df96da8
* https://github.com/rdesktop/rdesktop/commit/87d8d123b81093ec8ce635e3c5009038e5a102f3#diff-5931449ee480a798cabbfe5a0df96da8
* https://github.com/rdesktop/rdesktop/commit/908ad64d846502e8eecb887bb8509071b1c3ae38#diff-5931449ee480a798cabbfe5a0df96da8
* https://github.com/rdesktop/rdesktop/commit/5b7b955487a7f6886500aec6edcdc684da49e9d4#diff-5931449ee480a798cabbfe5a0df96da8

So here's what I suggest: take the logical changes in a3dfceef and apply them manually.  Re-generate a diff and call it patch-openssl3.  If you're uncomfortable doing that, let me know and I'd be happy to provide the proposed patch-openssl3 for you.
Comment 16 Jochen Neumeister freebsd_committer 2018-12-14 13:04:27 UTC
(In reply to Greg Fitzgerald from comment #5)

This fix me Problem with FreeBSD 12.0

http://joneumbox.org/build.html?mastername=120amd64-ports&build=2018-12-14_14h00m52s
http://joneumbox.org/build.html?mastername=120i386-ports&build=2018-12-14_14h00m54s

Can we committed this patch?
Comment 17 Greg Fitzgerald 2018-12-14 22:45:01 UTC
(In reply to Jochen Neumeister from comment #16)
As stated below my comments, this patch is incorrect and shouldn't be merged in it's current state. I haven't had the spare time to work on this so it's sitting till either I have time or someone steps up and does the work. Sorry for the delays!
Comment 18 Conrad Meyer freebsd_committer 2018-12-15 01:24:36 UTC
I'll throw something in phabricator today-ish.
Comment 19 Conrad Meyer freebsd_committer 2018-12-15 02:02:04 UTC
I've posted a review up at https://reviews.freebsd.org/D18566 .  It builds and basic functions seem fine.  I did not test thoroughly.
Comment 20 Jack 2019-01-18 22:59:17 UTC
Created attachment 201252 [details]
Upgrade rdesktop to 1.8.4

rdesktop 1.8.4 compiles fine and does not have the openssl error. Upgrade port to rdesktop 1.8.4
Comment 21 Jack 2019-01-18 23:05:29 UTC
Created attachment 201253 [details]
Log of rdesktop 1.8.4 building with openssl 1.1.1a on FreeBSD 12-STABLE
Comment 22 Greg Fitzgerald 2019-01-23 13:09:12 UTC
Created attachment 201358 [details]
rdesktop-1.8.4.diff

This is my working patch for 1.8.4
Comment 23 Yuri Pankov freebsd_committer 2019-02-08 06:03:32 UTC
1.8.4 release seems to have lots of security fixes in addition to openssl 1.1.1 build ones (https://github.com/rdesktop/rdesktop/releases/tag/v1.8.4), could we please go forward with this one?
Comment 24 Walter Schwarzenfeld freebsd_triage 2019-02-08 15:04:01 UTC
*** Bug 235600 has been marked as a duplicate of this bug. ***
Comment 25 Jochen Neumeister freebsd_committer 2019-02-08 15:24:11 UTC
Which is the right patch? Please delete all others.
That's just some chaos here
Comment 26 Jochen Neumeister freebsd_committer 2019-02-15 21:05:26 UTC
ping
Comment 27 Greg Fitzgerald 2019-02-15 21:29:14 UTC
The one made by me. I don't think I have permissions to obsolete others patches.
Comment 28 Walter Schwarzenfeld freebsd_triage 2019-02-20 16:11:52 UTC
See also: bug #235885.
Comment 29 Greg Veldman 2019-02-20 16:27:18 UTC
In attachment 201358 [details] if you change USES=autoreconf:build to just USES=autoreconf you shouldn't need the pre-configure stanza.
Comment 30 commit-hook freebsd_committer 2019-02-22 06:34:29 UTC
A commit references this bug:

Author: pi
Date: Fri Feb 22 06:34:05 UTC 2019
New revision: 493554
URL: https://svnweb.freebsd.org/changeset/ports/493554

Log:
  net/rdesktop: update 1.8.3 -> 1.8.4

  - many more CVEs are fixed by this upgrade, see Relnotes

  PR:		235885, 229029
  Submitted by:	Greg Veldman <freebsd@gregv.net>
  Reviewed by:	w.schwarzenfeld@utanet.at, brnd, cem, joneum
  Approved by:	gregf@hugops.pw (maintainer timeout)
  Relnotes:	https://github.com/rdesktop/rdesktop/releases/tag/v1.8.4
  Security:	CVE-2018-8794
  MFH:		2019Q1
  Differential Revision:	https://reviews.freebsd.org/D18566

Changes:
  head/net/rdesktop/Makefile
  head/net/rdesktop/distinfo
  head/net/rdesktop/files/patch-configure
  head/net/rdesktop/pkg-plist
Comment 31 commit-hook freebsd_committer 2019-02-22 08:41:12 UTC
A commit references this bug:

Author: pi
Date: Fri Feb 22 08:40:58 UTC 2019
New revision: 493562
URL: https://svnweb.freebsd.org/changeset/ports/493562

Log:
  MFH: r493554

  net/rdesktop: update 1.8.3 -> 1.8.4

  - many more CVEs are fixed by this upgrade, see Relnotes

  PR:		235885, 229029
  Submitted by:	Greg Veldman <freebsd@gregv.net>
  Reviewed by:	w.schwarzenfeld@utanet.at, brnd, cem, joneum
  Approved by:	gregf@hugops.pw (maintainer timeout)
  Relnotes:	https://github.com/rdesktop/rdesktop/releases/tag/v1.8.4
  Security:	CVE-2018-8794
  Differential Revision:	https://reviews.freebsd.org/D18566
  Approved by:	ports-secteam (joneum)

Changes:
_U  branches/2019Q1/
  branches/2019Q1/net/rdesktop/Makefile
  branches/2019Q1/net/rdesktop/distinfo
  branches/2019Q1/net/rdesktop/files/patch-configure
  branches/2019Q1/net/rdesktop/pkg-plist
Comment 32 commit-hook freebsd_committer 2019-02-22 17:58:38 UTC
A commit references this bug:

Author: pi
Date: Fri Feb 22 17:58:17 UTC 2019
New revision: 493578
URL: https://svnweb.freebsd.org/changeset/ports/493578

Log:
  security/vuxml: dokument rdesktop < 1.8.4 vulnerabilities

  PR:		235885, 229029

Changes:
  head/security/vuxml/vuln.xml
Comment 33 Kurt Jaeger freebsd_committer 2019-02-22 17:59:54 UTC
Committed, thanks to all!