GNOME Bugzilla – Bug 745890
[review] handling WEP keys in NMSettingWirelessSecurity [th/wep-key-bgo745890]
Last modified: 2015-03-20 12:17:17 UTC
- WEP keys must not invalidate a connection - try to detect the WEP key type for UNKNOWN
branch th/wep-key-bgo745890
Should libnm-util get the same change? The rest looks OK to me.
(In reply to Dan Williams from comment #2) > Should libnm-util get the same change? > > The rest looks OK to me. I think yes. I also extended the patch to the psk and leap_password fields. Repushed.
Hmm, I think we should still check for "\0" for all the passwords though, that is clearly invalid in all cases. Even if that was OK and would work, it's pointless to try a "\0" password and then re-request secrets.
(In reply to Dan Williams from comment #4) > Hmm, I think we should still check for "\0" for all the passwords though, > that is clearly invalid in all cases. Even if that was OK and would work, > it's pointless to try a "\0" password and then re-request secrets. AFAIS, nothing checks that requesting-secrets doesn't update the password with empty strings. Hence, the user entering an empty password could make a connection invalid.
(In reply to Thomas Haller from comment #5) > (In reply to Dan Williams from comment #4) > > Hmm, I think we should still check for "\0" for all the passwords though, > > that is clearly invalid in all cases. Even if that was OK and would work, > > it's pointless to try a "\0" password and then re-request secrets. > > AFAIS, nothing checks that requesting-secrets doesn't update the password > with empty strings. Hence, the user entering an empty password could make a > connection invalid. I'd argue that a connection with a NUL password is an invalid connection, because that should never happen, because we know it's going to fail. But we also don't have a way to clear out old secrets either. For example, if you set wep-key0 in the first request, but it turns out it's supposed to be wep-key1, the agent has no way of indicating that wep-key0 should be cleared because D-Bus doesn't allow passing NULL strings. Perhaps we should re-purpose NUL (eg, \0) as NULL in update_one_secret()? Anyway, that's kinda side-point. I just don't think allowing NULL secrets are useful, and I think we should reject them (or ignore them) when updating the connection's secrets.
(In reply to Dan Williams from comment #6) > (In reply to Thomas Haller from comment #5) > > (In reply to Dan Williams from comment #4) > > > Hmm, I think we should still check for "\0" for all the passwords though, > > > that is clearly invalid in all cases. Even if that was OK and would work, > > > it's pointless to try a "\0" password and then re-request secrets. > > > > AFAIS, nothing checks that requesting-secrets doesn't update the password > > with empty strings. Hence, the user entering an empty password could make a > > connection invalid. > > I'd argue that a connection with a NUL password is an invalid connection, > because that should never happen, because we know it's going to fail. That is not the point. NM has the assumption that all the connection in ((NMSettingsPrivate *)priv)->connections verify. For example, keyfiles _internal_write_connection() asserts that the connection verifies. When updating secrets, we must make sure that the connection still verifies afterwards. The easiest way to do that, is not to consider secrets in verify(). Also, checking secrets is not that useful. It means that you cannot load a keyfile with the following: [wifi-security] wep-key0= wep-key-type=0 although, when you allow it, NM can re-ask for a password as needed. The difference between other settings and secrets is, that if non-secret settings are invalid, nothing can fix it and NM cannot ask the user for the correct setting. For secrets that is different, NM can re-ask. But we cannot re-ask if we reject the connection as invalid straight away. > But we also don't have a way to clear out old secrets either. For example, > if you set wep-key0 in the first request, but it turns out it's supposed to > be wep-key1, the agent has no way of indicating that wep-key0 should be > cleared because D-Bus doesn't allow passing NULL strings. Perhaps we should > re-purpose NUL (eg, \0) as NULL in update_one_secret()? maybe. But that's a different patch :) > Anyway, that's kinda side-point. I just don't think allowing NULL secrets > are useful, and I think we should reject them (or ignore them) when updating > the connection's secrets.
Yes, what you say is all true. However, I think we should just ignore empty/NUL secrets in each setting's set_property() function and possibly filter them out in nm_connection_update_secrets().
merged: master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=3ef2a5364b1494230f2135f8b642d3a0fd2b2f30 nm-1-0: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=844a9c0dcd6768da70150b1cd80cabda09bd5a96 (In reply to Dan Williams from comment #8) > Yes, what you say is all true. However, I think we should just ignore > empty/NUL secrets in each setting's set_property() function and possibly > filter them out in nm_connection_update_secrets(). Yes, maybe, in a second patch? But the important thing is that need_secrets() asks for new secrets if the password is invalid (be it NULL, or "", or !nm_utils_wep_key_valid()). Then it doesn't really matter whether priv->password is NULL otherwise invalid. That is fixed now too (it was wrong before for nm-setting-adsl.c and others). I think that is the summary: - verify() must not check for valid secrets. - need_secrets() must validate.