Bug 229029 - net/rdesktop Fails to build with OpenSSL 1.1
Summary: net/rdesktop Fails to build with OpenSSL 1.1
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Bernard Spil
URL:
Keywords:
Depends on:
Blocks: 228865
  Show dependency treegraph
 
Reported: 2018-06-14 20:18 UTC by Bernard Spil
Modified: 2018-12-02 18:29 UTC (History)
4 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

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.