Salt generation for refresh token encryption seems unstable (BadPaddingException)

[vilaureu]

Hello,

I regularly get the following error message when using DavMail.

davmail.exchange.auth.O365Token  - refresh token failed javax.crypto.BadPaddingException: Given final block not properly padded. Such issues can arise if a bad key is used during decryption.

I made sure several times that my key does not change contrary to what the error message suggests.
It seems to me that the code for encryption and decryption of the refresh token tires to derive a salt for the cryptography via the hostname.
However, the getCanonicalHostName() function seems to prioritize the reverse lookup of the IP address over the already-known hostname (from getLocalHost()).
As an example, when I execute the following code, I get one.one.one.one as the hostname instead of the expected myorg.
System.out.println(InetAddress.getByAddress("myorg", new byte[]{1, 1, 1, 1}).getCanonicalHostName());
I suspect, that my decryption issues happen, when I change connect to a different network where my IP address (for some unknown reason) has a canonical hostname registered that then gets used instead of my local hostname.
I checked that changing the salt results in the same error message as using the wrong password.
Maybe this was also the underlying issue of #234 . (The old issue maybe just resolved because the network setting changed.)

I think, the best solution would be to generate a random salt value and store it along side the token (maybe just in the beginning of the Base64 encoded ciphertext).
But, I guess, just always using the fallback davmailgateway!& or using getHostName() (this does not do a reverse lookup but just takes the local hostname; but was abandoned in 4ddaea0) might also be viable options.
I do not know how to change this without breaking existing setups but maybe one could introduce another prefix to the stored refreshToken (similar to the existing {AES}) to discriminate different methods of generating the salt.

I am using the newest DavMail version (6.3.0) from Flathub if that makes any difference.

[mguessan]

Nice catch, was wondering why I got some random reauthentication after laptop waking up from sleep and changing location.

The main reason to switch to getCanonicalHostName was to have a chance to make it 16 characters long, we can just concatenate hostname with default fingerprint.

=> trying this in trunk

[Robin-Sch]

I have been playing with my davmail-in-docker setup. As described #381 (comment), it should be possible to generate a refreshToken on a system with a GUI, e.g., my PC, and then copy it over to my headless instance. For some reason I can not get this to work anymore. I can do everything on my PC and if I connect on davmail on my PC everything is fine, but as soon as I try to connect to my vps (with the refreshToken copied over), I get the same error as described here above.

More details and exact Dockerfile I am using: https://github.com/Robin-Sch/davmail-docker

ERROR [ImapConnection-57538] davmail.exchange.auth.O365Token  - refresh token failed javax.crypto.BadPaddingException: Given final block not properly padded. Such issues can arise if a bad key is used during decryption.
Please open the following link:
https://login.microsoftonline.com/common/oauth2/authorize?client_id=....
 proceed through authentication steps and paste back the final url that contains authentication code (blank page)
Authentication code: 2025-06-06 20:09:13,772 ERROR [ImapConnection-57538] davmail.exchange.auth.O365ManualAuthenticator  - Authentication failed, code not available
ERROR [ImapConnection-57538] davmail  - Authentication failed null
davmail.exception.DavMailException: Authentication failed null
 	at davmail.exchange.auth.O365ManualAuthenticator.authenticate(O365ManualAuthenticator.java:119)
 	at davmail.exchange.ExchangeSessionFactory.getInstance(ExchangeSessionFactory.java:183)
 	at davmail.exchange.ExchangeSessionFactory.getInstance(ExchangeSessionFactory.java:94)
	at davmail.imap.ImapConnection.run(ImapConnection.java:128)

EDIT: I also tried using trunk and also 00f78d2a92822fe244aa8f39fd3aa04efc6500bd (latest commit of 2024, which is way before I tried to run davmail in docker last time, so should be working). However, even with that commit it gives the same error, so I guess something is up with my new docker setup. Would you mind quickly going through it, maybe you can find the issue?

EDIT2: I also tried copying the newly generated refreshToken from my PC to the old docker setup, which gives the same error. Is it possible something changed recently that made the refreshToken only work per machine?

I saw there were some talks about the hostname being used as a salt?, for that reason I set the hostname of the container the same so that shouldn't be an issue. Also after restarting the container on my pc it still works, so hm....

[Robin-Sch]

Okay I managed to get it to work. I copied over the access token that was generated a few months ago and that is working with the trunk branch, however, it is not working with master. So I guess somewhere between trunk and master the access token format/generation got changed which does not allow it to be used on different machines?

[esabol]

@Robin-Sch wrote:

Okay I managed to get it to work. I copied over the access token that was generated a few months ago and that is working with the trunk branch, however, it is not working with master. So I guess somewhere between trunk and master the access token format/generation got changed which does not allow it to be used on different machines?

Well, there are 105 commits between trunk and master:

trunk...master

Can you narrow it down by testing individual commits using a binary search?

Or maybe just read through the commits and see which ones look like they might have something to do with tokens or encryption?

[esabol]

Actually, it's kind of obvious from reading this issue, isn't it? The commit is mentioned above:

997231f

[Robin-Sch]

Actually, it's kind of obvious from reading this issue, isn't it? The commit is mentioned above:

997231f

Yes, you are completely right! However, if I now try to generate a new access token I can not use it on my server.

If I generate a token on master on my PC, then if I try to use this token on master on my server it gives the error mentioned above
If I generate a token on trunk on my PC, then if I try to use this token on trunk on my server it also gives that error (huh?...)

Generated on machine - branch	Used on machine - branch	Expected	Works
PC - master	PC - master	V	V
PC - master	Server - master	V	X
PC - trunk	PC - trunk	V	V
PC - trunk	Server - trunk	V	X
PC - unknown (before trunk)	Server - master	X	X
PC - unknown (before trunk	Server - cd143c4 (one before 997231f)	V	V
PC - unknown (before trunk)	Server - trunk	V	V

[esabol]

997231f changed the StringEncryptor class, so no encrypted string generated before that change will work with any version of the code after that change, presumably.

Besides the change to include the "default fingerprint" string as a fallback, the StringEncryptor code previously used getCanonicalHostName() and now it uses getHostName(). The former returns a hostname like host.example.com. The latter returns an IP address. So I think the result of that change is that you need to encrypt and decrypt the string on machines with the same IP address.

So for the PC - master | Server - master case, I think you will need InetAddress.getLocalHost().getHostName() to return the same value (i.e. have the same IP address) on both machines ("PC" and "Server") in order for it to work . Since you are running in a Docker container, perhaps there is a way to make that happen? Check the answers here: https://stackoverflow.com/questions/27937185/assign-static-ip-to-docker-container

[esabol]

I think I agree with what @vilaureu wrote in the first post:

I think, the best solution would be to generate a random salt value and store it along side the token

Or add a configuration option to not use InetAddress.getLocalHost().getHostName() at all and always use the default fingerprint or perhaps a fingerprint of the user's choosing.

[mguessan]

Initial ticket submitter @vilaureu had an issue with getCanonicalHostName() that is supposed to return machine fqdn, problem is this fqdn can change based on machine location, e.g. a laptop at home vs at the office.
The other option is getLocalHost() that is supposed to return the short host name of the machine, but will sometimes return an IP, and for Docker return the instance id that will change each time we restart the instance

According to this it's possible to force docker hostname
https://stackoverflow.com/questions/52339884/how-to-return-localhost-from-inetaddress-getlocalhost-gethostname-in-docke

However it seems that this random fingerprint generation is overall not reliable and an issue for users that want to create a token on a machine and transfer it to another machine.

Need to decide if we shouldn't just drop location based fingerprint completely and use a static value.
Someone with access to token file can easily determine salt value anyway.

[esabol]

I'm OK with using a static value, but I still think there should be a property that a user can set that specifies which value to use. It would be at least as secure as the permissions on the property file then. And people who want to use some number of characters from /dev/urandom or similar to generate their own key can do that.

If you want to get fancy, you could check the key specified in the properties file to see if it has ${HOSTNAME} or ${IPADDRESS} contained in the string and do a replacement of those substrings with the output of the appropriate Java functions. But I'm not sure what that would gain exactly other than allowing users to control better how the key is generated (and moving it into a property already effectively does that).