> 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
Created attachment 194639 [details]
Created attachment 194640 [details]
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?
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.
Created attachment 196387 [details]
Reporter is committer, assign accordingly.
Everyone busy or am I supposed to be doing something here, I see it needs maintainer feedback, but I'm not sure on what.
I need this port :-).
I'm happy to test and/or commit, but I need approval from someone with a ports bit.
(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:
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.
(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.
No rush for me anymore — vangyzen@ pointed me at Remmina as a suitable replacement.
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.
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.
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.
(In reply to Conrad Meyer from comment #13)
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.
(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:
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:
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.