Security: Implement perfect forward secrecy for MTProto

[habcawa]

Hi!

Thanks for the great work with Telethon.

Quite a while ago I have implemented support for perfect forward secrecy in MTProto (cf. https://core.telegram.org/api/pfs) and would love to share this work with the community.

I understand that v1 is feature frozen, however, I think that this provides an advantage in security for all users and is not a new feature per se but rather a security fix.
This PR also fixes two bugs (type confusion, error handling on seq_no too low/too high) that can cause issues.

I have tested this with both a personal account and a bot token and did not encouter any problems.
I have also tested the behavior on the expiration of a temporary auth key.

[Lonami]

Looks good. It's much appreciated. My only worries are:

• If this breaks, I don't know if I'll be able to provide a timely fix myself.
• There'll likely be an expectation that v2 (if I ever finish that) will support PFS too.

I'm assuming the performance hit is not something we should worry about, since by default they expire after a day (although the first connection might be noticeably slower, specially with pure-Python encryption on mobile devices).

[andrew-ld]

there are shortcomings in this implementation.

first it does not handle the -404 error code at the transport level, telegram clearly says that temporary keys could be deleted from the server before they expire, you have to regenerate the key in this case

the AUTH_KEY_PERM_EMPTY error should also be handled, sometimes telegram may forget which persistent key the temporary key is bound to, again the key must be regenerated.

in the case of -404 if the key was generated within 60 seconds of the error you simply have to reconnect instead of regenerating it because it happens that immediately after generating it the server still does not accept the key.

[habcawa]

Looks good. It's much appreciated. [...]

I am happy to hear! Thanks, also for your comments. I'll provide a patch shortly.

• If this breaks, I don't know if I'll be able to provide a timely fix myself.
• There'll likely be an expectation that v2 (if I ever finish that) will support PFS too.

At least for the latter issue, I can offer that I'll give implementing this in v2 a try in the next weeks.
I would presume that most of the heavy lifting is already done, but I have never worked with v2 and would need to get familiar with the codebase.

I'm assuming the performance hit is not something we should worry about, since by default they expire after a day (although the first connection might be noticeably slower, specially with pure-Python encryption on mobile devices).

I would think so, too. After the temporary key has been generated and bound, performance should be back to normal until the key needs to be changed.

[habcawa]

Thanks for your review @andrew-ld.

the AUTH_KEY_PERM_EMPTY error should also be handled, sometimes telegram may forget which persistent key the temporary key is bound to, again the key must be regenerated.

I fixed this and will provide a patch shortly. I am not sure if this is the correct place, so please let me know about any further changes you would like to see. Accessing a private method is also not really a nice thing here.

first it does not handle the -404 error code at the transport level, telegram clearly says that temporary keys could be deleted from the server before they expire, you have to regenerate the key in this case
[...]
in the case of -404 if the key was generated within 60 seconds of the error you simply have to reconnect instead of regenerating it because it happens that immediately after generating it the server still does not accept the key.

Yes, I have also read that part of the docs. Therefore, the current implementation should always attempt to regenerate the key if I am not missing something. I have changed the following to test this:

diff --git a/telethon/network/authenticator.py b/telethon/network/authenticator.py
index e32df6f9..9507112b 100644
--- a/telethon/network/authenticator.py
+++ b/telethon/network/authenticator.py
@@ -44,7 +44,7 @@ async def do_authentication(sender, tmp_auth_key=False, tmp_auth_key_expires_s =
     new_nonce = int.from_bytes(os.urandom(32), 'little', signed=True)
 
     if tmp_auth_key:
-        expires_at = int(time.time()) + tmp_auth_key_expires_s
+        expires_at = int(time.time()) + 10 # tmp_auth_key_expires_s
         pq_inner_data = bytes(PQInnerDataTemp(
             pq=rsa.get_byte_array(pq), p=p, q=q,
             nonce=res_pq.nonce,

If I run the following program, the expired key is handled, as expected, by reconnecting (which triggers the generation of a new key).

client = TelegramClient('a', api_id, api_hash)
async for dialog in client.iter_dialogs():
        await client.send_message(dialog.id, f'Hello, {dialog.name}! {time.time()}')
        print('Send 1 done')
        await asyncio.sleep(10)
        
        await client.send_message(dialog.id, f'Hello, {dialog.name} 2! {time.time()}')
        print('Send 2 done')
        await asyncio.sleep(2)
        break

As before, please let me know if I am missing something here.

[andrew-ld]

i dont think that always re-generate the temp auth key is a good idea, the client must keep the generated temp key in memory and re-generate only if needed

[habcawa]

i dont think that always re-generate the temp auth key is a good idea, the client must keep the generated temp key in memory and re-generate only if needed

Sorry for the late reply - could you please elaborate what changes you'd like to see in this regard?