Epiphany crashed writing a long reply and I don't want to retype it... summary:

Proposed API looks great. _disable_ciphersuite should call g_warning if it doesn't recognize the passed string, so users don't need to check the return value.

We should probably provide _enable versions if someone requests it. These make sense if used to respond to explicit user configuration. GNOME would surely not need them. I don't see the need to provide _enable versions until somebody complains. It's obviously not a problem for anyone currently using the library, or they haven't told us if it is. OTOH: _enable would not be much more work to provide.

No need to "allow more configuratibility than this" but I *do* want API to get at the settings, if not to set them. I want to be able to test for voluntary protocol fallback, DH parameter size, etc. etc. (See the paragraph on https://wiki.gnome.org/Apps/Web/Development/Security regarding the insecure lock icon.) Now it'd be nice to have a simple API (strawman: gboolean g_tls_connection_is_probably_secure) for this, which we could use if it determines security on lowest-denominator web browser terms -- so even if SSLv3 is enabled by default in glib-networking the function would return false if it's negotiated -- but I'd also like to be able to display technical details to users who'd otherwise think we're worse than Firefox because Firefox "knows" the site is secure. The API for that would obviously need to be more complex. OTOH: once we have _get versions, _set versions would not be much more work to provide.

Related, but different issue: I interpret the Fedora crypto policy differently: I think it prohibits disabling ciphersuites by default, as well as enabling them. (And changing other settings like %LATEST_RECORD_VERSION. And probably our voluntary protocol fallback as well.) The point is that users must configure the app for it it use different settings. Regardless of which of us is right about that, we are not currently in compliance because we use NORMAL and not @SYSTEM as our base. We can't change that upstream because @SYSTEM is a downstream thing. We really shouldn't use NORMAL upstream because it's not secure. We definitely can't use @SYSTEM downstream, as required, because it allows SSLv3. (Well you can for glib-networking, but we'd still need to override it in WebKit.) I don't know what we should do about this; the status quo (just ignore the policy) seems like the best option, but we shouldn't do that. Probably need to chat with Nikos.

(In reply to Michael Catanzaro from comment #1)
> No need to "allow more configuratibility than this" but I *do* want API to
> get at the settings, if not to set them.

I have a branch with:

GTlsVersion  g_tls_connection_get_version           (GTlsConnection *conn);
const char  *g_tls_connection_get_version_name      (GTlsConnection *conn);
guint16      g_tls_connection_get_cipher_suite      (GTlsConnection *conn);
const char  *g_tls_connection_get_cipher_suite_name (GTlsConnection *conn);

get_cipher_suite() returns raw protocol values (eg, 0x003D), get_cipher_suite_name() returns the names ("TLS_RSA_WITH_AES_256_CBC_SHA256"). Though actually there's probably no reason to have the numeric version.

(get_version_name() seemed potentially-useful since if your gnutls is newer than your glib, get_version() might end up returning a value that doesn't match any known value of GTlsVersion. OTOH, what could you really do programmatically with a stringified version name?)

> I want to be able to test for voluntary protocol fallback

GLib itself does not do fallback. That's implemented at the libsoup level by setting GTlsConnection:use-ssl3 on the connection, so you can check for that. (I may add :use-fallback as an alias for that, since it actually means "use TLS 1.0" in epiphany's case now.)

> DH parameter size

That's gnutls_dh_get_prime_bits()? I suppose we'd want to support setting this on the server side as well...

> etc. etc. (See the paragraph
> on https://wiki.gnome.org/Apps/Web/Development/Security regarding the
> insecure lock icon.)

>> if RSA key exchange was used (no forward secrecy)
>> if we negotiated RC4

both of those can be determined from g_tls_connection_get_cipher_suite_name()

>> if a non-root cert has a SHA-1 signature
>> if any cert in the chain has an RSA 1024 key

Those would be GTlsCertificate APIs, not GTlsConnection. Bug 711864 is a very high-level "we need to expose more info about certificates" bug.

However, also, it has always been the intention that apps that want to know more details about certificates than we provide could just link to gnutls/nss/etc themselves and re-parse the certs from the raw data in their :certificate property.

>> if the server didn't send the safe renegotiation extension, 

That's gnutls_safe_renegotiation_status()? Though it might make more sense to provide "g_tls_connection_get_negotiated_extensions()" or something like that.

> Now it'd be nice to have a simple API (strawman:
> gboolean g_tls_connection_is_probably_secure) for this, which we could use
> if it determines security on lowest-denominator web browser terms

So there are two problems with that: (1) it makes an implicit claim of cryptographic savviness on the part of the GLib maintainers that we can't actually back up, and (2) "probably secure" is a moving target, so either the function would sometimes be wrong, or else we'd have to put out security bugfix releases every time another exploit came out. So I don't think this makes sense at the GLib level.

(One way it could make sense would be if it was tied into something like the Fedora system-wide crypto policy stuff; if there was some file on the system that defined what "probably secure" meant, then GLib could check if the connection met the criteria listed there. And then when new exploits came out, distros/admins would just update that file, rather than updating GLib.)

> Related, but different issue: I interpret the Fedora crypto policy
> differently: I think it prohibits disabling ciphersuites by default, as well
> as enabling them.

It does technically imply that, yes, but I don't think that was actually intended.

(In reply to Dan Winship from comment #2)
> (get_version_name() seemed potentially-useful since if your gnutls is newer
> than your glib, get_version() might end up returning a value that doesn't
> match any known value of GTlsVersion. OTOH, what could you really do
> programmatically with a stringified version name?)

I'm not sure that'd be useful. I think "nothing" is the right answer. The documentation for _get_version should be very clear that it can return values that don't correspond to the enum type; I think it's the right thing to do here, but could be super confusing.

> GLib itself does not do fallback. That's implemented at the libsoup level by
> setting GTlsConnection:use-ssl3 on the connection, so you can check for
> that. (I may add :use-fallback as an alias for that, since it actually means
> "use TLS 1.0" in epiphany's case now.)

An alias would indeed be good. :)

> That's gnutls_dh_get_prime_bits()? I suppose we'd want to support setting
> this on the server side as well...

> both of those can be determined from g_tls_connection_get_cipher_suite_name()

Yes... yes....
  
> Those would be GTlsCertificate APIs, not GTlsConnection. Bug 711864 is a
> very high-level "we need to expose more info about certificates" bug.
> 
> However, also, it has always been the intention that apps that want to know
> more details about certificates than we provide could just link to
> gnutls/nss/etc themselves and re-parse the certs from the raw data in their
> :certificate property.

That's true...

One more thing I would want to do is display the chain of certificates, so we can show exactly what point in the chain the trust is broken. (But that is a far off future project.)

> That's gnutls_safe_renegotiation_status()?

I think so, yes. Note that connections without safe renegotiation can still be secure if the server disallows all renegotiations, but I still want to penalize servers that don't send the extension since the client cannot know it's safe without actually trying to renegotiate....

> Though it might make more sense
> to provide "g_tls_connection_get_negotiated_extensions()" or something like
> that.

Yes, maybe! OTOH it might make sense to have separate API for security-sensitive issues like safe renegotiation.

Furthermore this is a special case really, as we already have GTlsRehandshakeMode, which is super confusing: G_TLS_REHANDSHAKE_SAFELY is the default and will trick programmers into thinking that their client is... well, safe, but that does not block communication from servers that fail to send the safe renegotiation extension, so that enum only really affects security if used by the server. (If the server does not send that extension and does not completely block all renegotiation, then the connection is vulnerable. The client never sees the malicious renegotiation.) But if you were to add API to block potentially insecure servers (it would use the %SAFE_RENEGOTIATION priority string; the default is %PARTIAL_RENEGOTIATION which is a safe choice for servers but not clients) I would not be able to use it for compatibility reasons, unfortunately.
 
> (One way it could make sense would be if it was tied into something like the
> Fedora system-wide crypto policy stuff; if there was some file on the system
> that defined what "probably secure" meant, then GLib could check if the
> connection met the criteria listed there. And then when new exploits came
> out, distros/admins would just update that file, rather than updating GLib.)

That sounds better than each app deciding for itself what is secure and what is not.

> It does technically imply that, yes, but I don't think that was actually
> intended.

Hopefully not, because the problem is much easier if your interpretation is correct. :)

Adding my use case here since Dan pointed me to this bug when I asked on IRC.

I'd like to be able to access gnutls_cipher_get_key_size

This is needed to properly set up a SASL server with sasl_setprop(conn, SASL_SSF_EXTERNAL, &ssf)

I assume you only need to be able to get the key size of the cipher negotiated on a particular connection, not of arbitrary ciphersuites as in the gnutls API?

(And I had been thinking you could get this info from the ciphersuite name, but the DES/3DES suites don't include the key size in the name, since it's not configurable I guess.)

(In reply to Dan Winship from comment #5)
> I assume you only need to be able to get the key size of the cipher
> negotiated on a particular connection, not of arbitrary ciphersuites as in
> the gnutls API?
> 

Yes, that's correct.

So here's what I'm currently thinking:

/**
 * g_tls_connection_get_connection_info:
 * @conn: a #GTlsConnection
 *
 * Gets information about the encryption and other TLS session
 * parameters of @conn, as a #GVariant dictionary containing various
 * pieces of information.
 *
 * Certain pieces of information are available for all connections:
 *
 * - `version` (`q`): the negotiated #GTlsVersion. (This is just the
 *   version number value in the binary format used by the TLS
 *   protocol. For known SSL/TLS versions, this will be one of the
 *   values of the #GTlsVersion enumeration, but future versions of
 *   the backend TLS library may negotiate connections using TLS
 *   versions not yet known to GLib.)
 * - `key-exchange` (`s`): the key exchange algorithm (eg, "RSA" or
 *   "ECDH_ECDSA")
 * - `cipher` (`s`): the cipher algorithm (eg, "DES" or
 *   "AES_256_CBC")
 * - `mac` (`s`): the MAC algorithm (eg, "MD5" or "SHA256")
 * - `cipher-suite` (`s`): the full cipher suite name (eg,
 *   "TLS_RSA_WITH_AES128_CBC_SHA")
 * - `key-size` (`i`): the size of the key used by the cipher
 *   algorithm
 * - `mac-size` (`i`): the output size of the MAC algorithm
 *
 * Additional data may also be available depending on the ciphersuite
 * or extensions:
 *
 * - `dh-prime-size` (`i`): for cipher suites using Diffie-Hellman key
 *   exchange, the length in bits of the prime modulus.
 * - `ext-renegotiation-info` (`b`): %TRUE if client and server
 *   both support the TLS Renegotiation Indication Extension
 *
 * Additional items may be added in the future.
 *
 * Returns: (transfer full) (nullable): a variant dictionary
 * containing information about @conn, or %NULL if @conn is not
 * connected or has not completed a handshake.
 *
 * Since: 2.46
 */


/**
 * g_tls_connection_get_connection_requirements:
 * @conn: a #GTlsConnection
 *
 * Gets information about the available/permitted encryption and other
 * TLS session parameters of @conn, as a #GVariant dictionary
 * containing various pieces of information.
 *
 * The parameters are similar to those in
 * g_tls_connection_get_connection_info():
 *
 * - `min-version` (`q`): minimum allowed #GTlsVersion
 * - `max-version` (`q`): maximum allowed #GTlsVersion
 * - `key-exchange` (`as`): allowed key exchange algorithms
 * - `cipher` (`as`): allowed cipher algorithms
 * - `mac` (`as`): allowed MAC algorithms
 * - `cipher-suite` (`as`): allowed cipher suites
 * - `min-key-size` (`i`): minimum allowed cipher key size
 * - `min-mac-size` (`i`): minimum allowed MAC output size
 * - `min-dh-prime-size` (`i`): minimum DH prime modulus length
 *
 * Additional items may be added in the future.
 *
 * Returns: (transfer full) (nullable): a variant dictionary
 * containing information about @conn's requirements.
 *
 * Since: 2.46
 */


/**
 * g_tls_connection_set_connection_requirements:
 * @conn: a #GTlsConnection
 * @reqs: a vardict containing requirements for @conn
 *
 * Sets information about the permitted encryption and other TLS
 * session parameters of @conn.
 *
 * The parameters are as with
 * g_tls_connection_get_connection_requirements(), with a few
 * additional possibilities:
 *
 * - `min-version` (`q`): minimum #GTlsVersion
 * - `max-version` (`q`): maximum #GTlsVersion
 * - `key-exchange` (`as`): allowed key exchange algorithms
 * - `not-key-exchange` (`as`): forbidden key exchange algorithms
 * - `cipher` (`as`): allowed cipher algorithms
 * - `not-cipher` (`as`): forbidden cipher algorithms
 * - `mac` (`as`): allowed MAC algorithms
 * - `not-mac` (`as`): forbidden MAC algorithms
 * - `cipher-suite` (`as`): allowed cipher suites
 * - `not-cipher-suite` (`as`): forbidden cipher suites
 * - `min-key-size` (`i`): minimum cipher key size
 * - `min-mac-size` (`i`): minimum MAC output size
 * - `min-dh-prime-size` (`i`): minimum DH prime modulus length
 *
 * Parameters which are not present in @reqs will not have their
 * requirements changed.
 *
 * For the algorithm arrays, setting the "positive" parameter (eg,
 * `cipher`) will forbid any algorithms not listed, and setting the
 * "negative" parameter (`not-cipher`) will allow any algorithms not
 * listed. (It is a programmer error to specify both positive and
 * negative versions of the same parameter, or to specify both
 * `cipher-suite` and any of `key-exchange`, `cipher`, and `mac` (or
 * the corresponding `not-` parameters.)
 *
 * Unrecognized parameters will be ignored.
 *
 * The system may have policies in place that override this function.
 * Additionally, setting #GTlsClientConnection:use-ssl3 will override
 * the `min-version` and `max-version` requirements. You can call
 * g_tls_connection_get_connection_requirements() to see the actual
 * result of this call.
 *
 * Since: 2.46
 */

LGTM.

(In reply to Dan Winship from comment #7)
>  * The system may have policies in place that override this function.

Like what? I would assume that the @SYSTEM priority string and G_TLS_GNUTLS_PRIORITY environment variables provide the base priority, and that g_tls_connection_set_connection_requirements() would modify this priority.

>  * Additionally, setting #GTlsClientConnection:use-ssl3 will override
>  * the `min-version` and `max-version` requirements. You can call
>  * g_tls_connection_get_connection_requirements() to see the actual
>  * result of this call.

I think we should deprecate that property.

In the docs it is probably worth mentioning if the sizes are in bytes or bits since IIRC openssl and gnutls use different conventions

(In reply to Michael Catanzaro from comment #8)
> >  * The system may have policies in place that override this function.
> 
> Like what? I would assume that the @SYSTEM priority string and
> G_TLS_GNUTLS_PRIORITY environment variables provide the base priority, and
> that g_tls_connection_set_connection_requirements() would modify this
> priority.

My intention was that it would not be possible to enable a version/algorithm/suite that was disabled by @SYSTEM

> >  * Additionally, setting #GTlsClientConnection:use-ssl3 will override

> I think we should deprecate that property.

Yes

(In reply to Dan Winship from comment #10)
> My intention was that it would not be possible to enable a
> version/algorithm/suite that was disabled by @SYSTEM

Hm... I think apps should be able to enable things that are disabled by @SYSTEM. For instance, RC4 is disabled by @SYSTEM in Fedora. But Firefox wants to allow RC4 connections after displaying an interstitial security warning, for compatibility. I think the point of @SYSTEM is to ensure apps are using sane defaults so things are safe out-of-the-box, not to prevent configuration.

(In reply to Paolo Borelli from comment #9)
> In the docs it is probably worth mentioning if the sizes are in bytes or
> bits since IIRC openssl and gnutls use different conventions

Key sizes would surely be bits, but that would be good to mention, indeed.

Well, I want to avoid people *accidentally* making things less secure. Eg, given the current API proposal, if @SYSTEM disabled RC4, and you did set_requirements({ 'not-cipher': [ '3DES-CBC', 'DES-CBC' ] }), and @SYSTEM doesn't override that, then you'd end up enabling RC4.

Maybe the fix is that @SYSTEM should override not-foo, but not override foo. AKA, "cipher" means "allow all the ciphers listed", but "not-cipher" means "allow all the ciphers except the ones listed and the ones that are disabled by default". So the above would not enable RC4, but set_requirements({ 'cipher': [ 'RC4', ... ] }) would.

Although that would require explicitly re-listing all of the already-allowed ciphers along with RC4. Maybe there should be +/- syntax:

  /* allow default ciphers plus RC4 */
  { 'cipher': [ '+RC4' ] }

  /* allow default ciphers except 3DES-CBC and DES-CBC */
  { 'cipher': [ '-3DES-CBC', '-DES-CBC' ] }

  /* allow only AES-128-GCM and AES-256-GCM */
  { 'cipher': [ 'AES-128-GCM', 'AES-256-GCM' ] }

(Of course it's still possible that your choices will get overridden if you try to enable algorithms that the underlying crypto library doesn't support.)

You're right, it's too easy to accidentally enable/disable things:

 * For the algorithm arrays, setting the "positive" parameter (eg,
 * `cipher`) will forbid any algorithms not listed, and setting the
 * "negative" parameter (`not-cipher`) will allow any algorithms not
 * listed.

^ I should have read closer. This is a recipe for disaster, TBH. :)

(In reply to Dan Winship from comment #12)
> Maybe the fix is that @SYSTEM should override not-foo, but not override foo.
> AKA, "cipher" means "allow all the ciphers listed", but "not-cipher" means
> "allow all the ciphers except the ones listed and the ones that are disabled
> by default". So the above would not enable RC4, but set_requirements({
> 'cipher': [ 'RC4', ... ] }) would.

I think this would be unnecessarily confusing.
 
> Although that would require explicitly re-listing all of the already-allowed
> ciphers along with RC4. Maybe there should be +/- syntax:

Yes, something like that would work, although some application is going to try { 'cipher': [ 'AES-128-GCM', 'AES-256-GCM', '+RC4' ] } so we'd want to make that an error. Actually, some application will also use { 'cipher': [ 'RC4' ] } when it wants { 'cipher': [ '+RC4' ] }. But I think this is still much easier to use properly than the original proposal.

FYI: I should have time to work on this "soon".

pushed wip/danw/tls-new-apis (glib) and wip/danw/tls-info (glib-networking) with the work I'd done on this. (Basically-working g_tls_connection_get_connection_info(), and at least some of the others.) The glib branch actually has bits of some other stuff in it too (corresponding to wip/danw/sni and wip/danw/alpn in glib-networking), but I don't remember the state of that code, so you'll probably want to strip it out for now.

Thanks, I'll take a look, sometime in the next couple of weeks probably.

(In reply to Michael Catanzaro from comment #14)
> FYI: I should have time to work on this "soon".

Ha, ha

(In reply to Michael Catanzaro from comment #16)
> Thanks, I'll take a look, sometime in the next couple of weeks probably.

Hahahaha

Created attachment 366467 [details] [review]
tlsclientconnection: Update use-ssl3 documentation

The property documentation correctly indicates how this code works
nowadays, but the function documentation is obsolete and misleading.
Update it.

Created attachment 366468 [details] [review]
tlsclientconnection: Deprecate ssl3 property and functions

I originally planned to introduce a new property and functions to
replace these, with the same behavior but less-confusing names. But that
might not be the best approach in the long run. Instead, let's just
deprecate them without replacement.

TLS 1.2 intolerance is no longer a thing in the wild, and no known
GTlsBackend supports TLS 1.3 yet. But you might need to use this
property in the future, even though it's deprecated, if your
GTlsBackend has added support for TLS 1.3 and you need to talk to a
server that is TLS 1.3 intolerant.

Independently of all that, these APIs simply no longer do what their
names suggest, so deprecation is sensible regardless.

Wrong bug, sorry.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1000.

Reopening. This bug was migrated to https://gitlab.gnome.org/GNOME/glib, but it should have been migrated to https://gitlab.gnome.org/GNOME/glib-networking.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib-networking/issues/14.