paolostivanin / OTPClient

Highly secure and easy to use OTP client written in C/GTK3 that supports both TOTP and HOTP
GNU General Public License v3.0
481 stars 47 forks source link

Can't import from Aegis #369

Closed JonnyCodewalker closed 4 months ago

JonnyCodewalker commented 4 months ago

When trying to import from Aegis the following error appears:

OTPClient v3.6.0 (flatpak) Aegis v3.1

paolostivanin commented 4 months ago

Thanks for reporting this. I'll have a look at it.

If you could provide a simple reproducer, that would be fantastic though :smile:

paolostivanin commented 4 months ago

I cannot reproduce the issue with my dataset. I'll push a fix in a dedicated branch, you'll have to compile it yourself and let me know whether it has helped.

paolostivanin commented 4 months ago

Could you please try the fix in this branch and let me know? Thanks!

JonnyCodewalker commented 4 months ago

I've tried the encrypted import with the fix and it worked. Afterwards I also tried the unencrypted aegis import, which still failed with the same message. At this point I think maybe aegis changed something in its unencrypted export and it is a general issue, but I am not sure.

The unencrypted aegis export is a .txt with the following format: otpauth://totp/name-stuff&period=30&digits=6&algorithm=SHA1&secret=blabla&issuer=name so maybe something changed there?

paolostivanin commented 4 months ago

I'll make the error message more clear, but the issue here is that you are importing an otpauth format, while the unencrypted version supports only the plain json :smile: I'll check whether I can add auto-detect :thinking:

paolostivanin commented 4 months ago

done, now it automatically detects whether it's a plain json or plain txt :smile:

now I've gotta test it

JonnyCodewalker commented 4 months ago

If you push it to the tryfix branch I am happy to test it aswell.

paolostivanin commented 4 months ago

done :smile:

JonnyCodewalker commented 4 months ago

Plaintext import crashes with:

(otpclient:168235): GLib-CRITICAL **: 10:11:39.944: g_strsplit: assertion 'string != NULL' failed
fish: Job 1, 'otpclient' terminated by signal SIGSEGV (Address boundary error)

Encrypted still works, so I don't think it is a memlock limit issue.

Full output:

  otpclient
[WARNING] your operating system's memlock limit may be too low for you (current value: 8388608 bytes).
This may cause issues when importing third parties databases or dealing with tens of tokens.
For information on how to increase the memlock value, please have a look at https://github.com/paolostivanin/OTPClient/wiki/Secure-Memory-Limitations
couldn't lock 16384 bytes of memory (secret_session): Cannot allocate memory
Gtk-Message: 10:11:14.312: Failed to load module "pk-gtk-module"
Backup copy successfully created.
Backup copy successfully created.
[WARNING] your operating system's memlock limit may be too low for you (current value: 8388608 bytes).
This may cause issues when importing third parties databases or dealing with tens of tokens.
For information on how to increase the memlock value, please have a look at https://github.com/paolostivanin/OTPClient/wiki/Secure-Memory-Limitations

(otpclient:168235): GLib-CRITICAL **: 10:11:39.944: g_strsplit: assertion 'string != NULL' failed
fish: Job 1, 'otpclient' terminated by signal SIGSEGV (Address boundary error)
paolostivanin commented 4 months ago
  1. this happend while loading the txt or the json?
  2. you've got a weird error, and the app didn't exit upon facing a failure in allocating the secmem. Very weird...
JonnyCodewalker commented 4 months ago
  1. The sigsev happens with the .txt, I don't even know if you still can export an unencrypted .json in Aegis. The first warning up to backup successful was the encrypted import, the second part the .txt. Though it also happened when using a new DB and only importing the txt, so it is at least consistent.
JonnyCodewalker commented 4 months ago

I assume that whatever null character that is present in my dataset and required the JSON_ALLOW_NULL is what now trips up the plaintext import.

Looking at the plaintext export it has the following entry: otpauth://totp/Bitwarden%3A%00name?period=30&digits=6&algorithm=SHA1&secret=secret&issuer=Bitwarden

Removing the %00 resolves the issue. I am not entirely sure how it entered the dataset.

Edit: Just checked, the username field in Aegis had a leading space in it. I do think OTPClient should be resilient to that, even if it is probably always a user input error.

paolostivanin commented 4 months ago
  1. what really puzzles me is that you got couldn't lock 16384 bytes of memory (secret_session): Cannot allocate memory and the app didn't exit. I don't understand why since I clearly exit if secmem fails to be allocated. I need to recheck the code.
  2. hmm interesting that you got a NUL char in there. That's what's definitely causing the issue since string parsing functions do check for null byte. I need to think how to detect such case
paolostivanin commented 4 months ago

but why is space encoded with %00? It should be encoded with %20 . That's an issue that should be reported to Aegis imho :thinking:

JonnyCodewalker commented 4 months ago

For what it is worth I am getting thecouldn't lock 16384 bytes of memory (secret_file_backend): Cannot allocate memory in the flatpak on fedora 40 as well as in the tryfix build inside a fedora 40 toolbx.

Regarding the white-space: I just tested by re-adding the space in front of the username and it is encoded as %20. My theory as to what happened is that, since this dataset has traveled with me through a few different 2FA apps at some point there was a conversion error/somehow the %20 turned into %00 or the %00 was added. Aegis managed to handle the fault but OTPClient tripped up. But that is just conjecture.

paolostivanin commented 4 months ago

I could find&replace %00 with %20 :thinking: I can't think of any other solution tbh.

Hmmm, interesting. Never used the flatpak version, so I've gotta check that too.

JonnyCodewalker commented 4 months ago

I could find&replace %00 with %20 🤔 I can't think of any other solution tbh.

Hmmm, interesting. Never used the flatpak version, so I've gotta check that too.

A simple find and replace might create issues if people copy paste their usernames and now there are spaces in it? Not sure if anyone actually does that though. Edit: Nevermind, I don't think you can even copy the usernames from OTPClient, so it should be good.

Should I create a separate issue for the secmem thing?

paolostivanin commented 4 months ago

Since the %00 should not be present in the otpauth format, I'll simply go ahead and remove it, if found.

Yes please, do create a separate issue! Thanks a lot!

paolostivanin commented 4 months ago

I've pushed a fix for the NUL char, could you please try it?

JonnyCodewalker commented 4 months ago

Fix seems to work, added a few %00 throughout the txt and it worked without issues. So now encrypted and unencrypted can handle the %00 fault.

:+1: