"Unable to determine the OTP code for this entry."

agboom commented 5 years ago

Hi, thanks for this great extension. It works like a charm, except when requesting an OTP token. I've filled out the issue form below as best as I could:

General information

Operating system + version: Arch Linux
Browser + version: Firefox Nightly (65.0a1)
Information about the host app:
- How did you install it? Pre-built binary from the GitHub releases page
- If installed an official release, put a version: 2.0.22
Information about the browser extension:
- How did you install it? Via Firefox Add-ons
- Browserpass extension version as reported by your browser: 2.0.22
Pass OTP version: 1.2.0

Exact steps to reproduce the problem

Create a pass entry with the format otpauth://totp/<user>?secret=<secret>?issuer=<issuer>?digits=6&period=30
Observe that pass otp from the command line works fine.
Request an OTP token from Browserpass by clicking on the OTP icon
See the error "Unable to determine the OTP code for this entry."

What should happen?

An OTP token should be shown

What happened instead?

The error "Unable to determine the OTP code for this entry." is shown.

Reproducing these steps while debugging in Firefox produced the following stacktrace:

Unable to determine the OTP code for this entry. script.js:430:3
    resetWithError moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:430
    loginToClipboard moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:332
    loginToClipboard moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:328
    callback moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1991
    updateEvent moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1995
    setAttr moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1899
    setAttrs moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1889
    createElement moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1516
    createNode moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1470
    createNodes moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1457
    createElement moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1529
    createNode moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1470
    createNodes moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1457
    updateNodes moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1658
    updateElement moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1739
    updateNode moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1681
    updateNodes moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1589
    updateElement moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1739
    updateNode moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1681
    updateComponent moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1753
    updateNode moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1684
    updateNodes moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1589
    render moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:2035
    run0 moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:2098
    throttle moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:2052
    redraw moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:2080
    searchPassword moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:272
    searchPassword moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:251
    searchPassword moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:249
    init moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:228
    [1]</</< moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:24
    [1]< moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:17
    o moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1
    r moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1
    <anonymous> moz-extension://bfc09d60-5780-42e5-a98a-062a858ba5b4/script.js:1

maximbaz commented 5 years ago

Ping @qbit & @erayd who I know use OTP functionality. We use github.com/gokyle/twofactor for OTP functionality and I'm sure the error is coming from there.

erayd commented 5 years ago

@agboom What's the exact length of the secret (in characters), and how many (if any) '=' characters does it have at the end?

agboom commented 5 years ago

@agboom What's the exact length of the secret (in characters), and how many (if any) '=' characters does it have at the end?

The secret is 32 characters long. It does not have = characters (except for the = in secret=, but that's not part of the secret I presume).

Thanks for replying so quickly!

agboom commented 5 years ago

I've taken the liberty to do some debugging myself, if it helps.

package main

import (
  "fmt"
  "github.com/gokyle/twofactor"
)

func main() {
  url := "otpauth://totp/ACME%20Co:john.doe@email.com?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=ACME%20Co&algorithm=SHA1&digits=6&period=30"
  totp, label, err := twofactor.FromURL(url)
  fmt.Println(totp)
  fmt.Println(label)
  fmt.Println(err)
  fmt.Println(totp.OTP())
}

Results in:

$ go run main.go
&{0xc000082000 30}
ACME Co:john.doe@email.com
<nil>
717622

I copied the OTP URI from here: https://github.com/google/google-authenticator/wiki/Key-Uri-Format#examples

The output seems to be alright.

qbit commented 5 years ago

If you put your secret in the example code does it produce the correct otp ?

agboom commented 5 years ago

Yes, it produces the same OTP as pass otp.

qbit commented 5 years ago

Are you sure your browserpass binary is the latest?

agboom commented 5 years ago

Yes, I upgraded to v2.0.22 before reproducing this issue.

[edit] The 2.0.21 binary had the same issue. The browser extension is at 2.0.22 too

qbit commented 5 years ago

2.0.21 was before I added the padding logic to twofactor. How are you installing the binary? Perhaps the deps were not up to date?

agboom commented 5 years ago

I installed the binary by downloading the zip file from the releases page and running install.sh. This placed the native messaging host browserpass-linux64 in ~/.mozilla/native-messaging-hosts.

qbit commented 5 years ago

So either your native-messaging-hosts file is pointed to the incorrect binary, or the 2.0.22 bin was built with an old version of twofactor.

maximbaz commented 5 years ago

Running install.sh does not place binary anywhere, it only creates a json file in ~/.mozilla/native-messaging-hosts containing the full path to the binary to use. Open that file, copy the full path, and execute /path/to/browserpass -v to see the version. Also double-check that you don't have a browserpass json file in /usr/lib/mozilla/native-messaging-hosts.

The latest binary has been built against v1.0.1:

https://github.com/browserpass/browserpass/blob/33d9be90d664362613f606914f603f8a9a0dcf3b/Gopkg.lock#L4-L8

agboom commented 5 years ago

@maximbaz You're right, I accidentaly misphrased.

The json file ~/.mozilla/native-messaging-hosts/com.dannyvankooten.browserpass.json exists and has a "path" key that has the value /home/<user>/.local/share/browserpass/browserpass-linux64, which points to the binary I extracted (my username is substituted for <user>).

The output of ~/.local/share/browserpass/browserpass-linux64 -v is:

Browserpass host app version: 2.0.22

There is no other browserpass file in /usr/lib/mozilla/native-messaging-hosts

maximbaz commented 5 years ago

Am I right to understand that you and @qbit tried the same example and only one of you reproduced the issue?

@agboom would you like to compile browserpass from sources to verify that the issue is not with the provided binary? Either use AUR package or follow CONTRIBUTING.md, both approaches should be fine. I verified now that browserpass-src.tar.gz that AUR package is using does contain the latest version of twofactor library.

agboom commented 5 years ago

@maximbaz I removed the manually installed browserpass binaries and reinstalled from the AUR package, version 2.0.21 since you pointed out that nothing has changed in 2.0.22 for Linux. I restarted Firefox and tried asking for the OTP code again, but the same issue occured.

I also installed 2.0.22 from the AUR, by editting the PKGBUILD, but that didn't change anything.

[edit] I also built from the latest commit, but sadly that also didn't change anything

agboom commented 5 years ago

I tried some other things:

The OTP URI was in a separate file. I put it in the same file as the username and password and tried again.
I created a new file in pass and put the OTP example from here in it and tried again.
I put otp: in front of the OTP URI.

All yield the same message I'm afraid: "Unable to determine the OTP code for this entry." Maybe I'm debugging in the wrong places?

qbit commented 5 years ago

@agboom just to confirm, when you put the otp url in the sample code you posted up above, it produces the proper otp, right?

agboom commented 5 years ago

@qbit That's correct. It produces the same OTP code as pass otp for the URI examples here and my own URI.

maximbaz commented 5 years ago

I'm curious, are you able to adapt the TestOtp method in browserpass_test.go so that it fails? (run tests with go test command in project root). Maybe we are looking for a problem in an incorrect place...

agboom commented 5 years ago

@maximbaz I tried your suggestion, but ran into a segmentation violation while running go test (on a clean repository). I opened a new ticket as it is a separate issue: #304

maximbaz commented 5 years ago

Fixed the bug, hopefully it unblocks you to continue your investigation. Looking forward to you nailing this down 😉

agboom commented 5 years ago

That's fast, thanks! Running go test works again. However, when I fill in either the example OTP URI or my own OTP URI in the test, it is able to produce an OTP code just fine. The assertions fail of course, but no unexpected errors.

agboom commented 5 years ago

The error that Browserpass shows comes from this line of code: https://github.com/browserpass/browserpass/blob/62bda2a62348be40ab64b2a603ae756fb5f7b869/chrome/background.browserify.js#L121

Maybe I can backtrace from there somehow?

maximbaz commented 5 years ago

Would be interesting if you inspect the contents of response variable, what it contains at the time of the error.

agboom commented 5 years ago

Happy Christmas! :christmas_tree:

I finally found the time to fire up the debugger. After placing a breakpoint on this line:

https://github.com/browserpass/browserpass/blob/62bda2a62348be40ab64b2a603ae756fb5f7b869/chrome/background.browserify.js#L118

The debugger paused there when clicking the OTP button and inspecting response gave the following value:

{
  "digits": "",
  "label": "",
  "p": "otpauth://totp/Example:alice@google.com?secret=JBSWY3DPEHPK3PXP&issuer=Example",
  "u": "",
  "url": ""
}

So it seems logical that the "Unable to determine..." response is shown, because response.digits is falsy. The next question is why it is falsy, but I couldn't figure it out immediately. I'll try some more things. Any pointers are much appreciated.

maximbaz commented 5 years ago

Does your entry have a password, or only URL? It is weird that OTP url is stored in the "p" field. Could you add a random string as the first line of the pass entry? If I'm right, we might have an assumption in the code that the first line must contain a password and the rest of the fields contain named metadata.

Merry Christmas 🎄

maximbaz commented 5 years ago

In general we can't break this assumption, we can't decide based on the entry content if the first line contains a password or labeled metadata, so we must reserve the first line for the password. If you don't need a password for this specific entry, put something in the first line, e.g. ---.

agboom commented 5 years ago

Ah, I wasn't aware of that assumption. I assumed that the behavior was equal to pass otp.

Interestingly, when I put --- on the first line and the OTP URI on the second, it gives a different error: "undefined". Upon debugging, I found that this error is returned at this line: https://github.com/browserpass/browserpass/blob/62bda2a62348be40ab64b2a603ae756fb5f7b869/chrome/script.browserify.js#L331

The response object is as follows:

{
  "error": "undefined",
  "status": "ERROR"
}

"undefined" seems to be the contents of chrome.runtime.lastError.message that is returned here:

https://github.com/browserpass/browserpass/blob/62bda2a62348be40ab64b2a603ae756fb5f7b869/chrome/background.browserify.js#L102

maximbaz commented 5 years ago

I'll document this assumption a bit later 👍

Can you post here the exact contents of the pass entry you are trying to use?

Also, can you try native.pl to make sure the native component alone works correctly?

$ /path/to/native.pl /path/to/browserpass '{"action": "get", "entry": "default:myentry"}'

agboom commented 5 years ago

The exact contents of the test entry:

---
otpauth://totp/Example:alice@google.com?secret=JBSWY3DPEHPK3PXP&issuer=Example

The native.pl action gives some more interesting insights, here are the results:

/home/user/repositories/native.pl/native.pl ./browserpass '{"action": "get", "entry": "default:test"}'
Request: |  {"action": "get", "entry": "default:test"}
Result:  |  -
Errors:  |  panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x5307d8]

goroutine 1 [running]:
browserpass/vendor/github.com/gokyle/twofactor.(*TOTP).OTPCounter(0xc000010630, 0x18)
    /home/user/repositories/go/src/browserpass/vendor/github.com/gokyle/twofactor/totp.go:56 +0x28
browserpass/vendor/github.com/gokyle/twofactor.(*TOTP).OTP(0xc000010630, 0x4e, 0x5a8660)
    /home/user/repositories/go/src/browserpass/vendor/github.com/gokyle/twofactor/totp.go:37 +0x2b
browserpass/vendor/github.com/dannyvankooten/browserpass.parseTotp(0xc0000220f0, 0x4e, 0xc00005c600, 0xffc, 0xc0000220f0)
    /home/user/repositories/go/src/browserpass/vendor/github.com/dannyvankooten/browserpass/browserpass.go:215 +0xce
browserpass/vendor/github.com/dannyvankooten/browserpass.parseLogin(0x5a74e0, 0xc00000e0b8, 0xc00000e0b8, 0x5a74e0, 0xc00000e0b8)
    /home/user/repositories/go/src/browserpass/vendor/github.com/dannyvankooten/browserpass/browserpass.go:239 +0x427
browserpass/vendor/github.com/dannyvankooten/browserpass.readLoginGPG(0x5a74e0, 0xc00000e090, 0x0, 0x0, 0x0)
    /home/user/repositories/go/src/browserpass/vendor/github.com/dannyvankooten/browserpass/browserpass.go:186 +0x1ba
browserpass/vendor/github.com/dannyvankooten/browserpass.Run(0x5a74e0, 0xc00000e010, 0x5a7500, 0xc00000e018, 0x0, 0x0)
    /home/user/repositories/go/src/browserpass/vendor/github.com/dannyvankooten/browserpass/browserpass.go:109 +0x775
main.main()
    /home/user/repositories/go/src/browserpass/cmd/browserpass/main.go:27 +0xc1

Adding a newline to the first line of the pass entry yields the same result, btw.

maximbaz commented 5 years ago

Weird I can't repro with neither browserpass from AUR nor from browserpass-linux64.zip of the latest release:

❯ ~/private/native.pl ./browserpass-linux64 '{"action": "get", "entry": "default:personal/temp"}' 
Request: |  {"action": "get", "entry": "default:personal/temp"}
Result:  |  {"u":"temp","p":"---","digits":"883711","label":"Example:alice@google.com","url":""}
Errors:  |  -

❯ ~/private/native.pl /usr/bin/browserpass '{"action": "get", "entry": "default:personal/temp"}'
Request: |  {"action": "get", "entry": "default:personal/temp"}
Result:  |  {"u":"temp","p":"---","digits":"524060","label":"Example:alice@google.com","url":""}
Errors:  |  -

agboom commented 5 years ago

Interesting, when I run make clean and make browserpass and try the native command again the issue does not occur anymore.

I ran a git bisect on the repo, running make clean, make browserpass and the native.pl command again, but couldn't find a bad commit. Everything works.

However, when I install browserpass-git from the AUR the segfault occurs again. The stacktrace seems to refer to a yaourt tmp directory that does not exist. See below for the stacktrace. A workaround for this problem is to use the stable package.

In any case, the original issue is solved by adding a --- line above the line with the OTP URI. The problem described above is a separate issue, so this can be closed.

/home/user/repositories/native.pl/native.pl /usr/bin/browserpass '{"action": "get", "entry": "default:test"}'
Request: |  {"action": "get", "entry": "default:test"}
Result:  |  -
Errors:  |  panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x5324b8]

goroutine 1 [running]:
github.com/gokyle/twofactor.(*TOTP).OTPCounter(0xc0000c6170, 0x18)
    /tmp/yaourt-tmp/aur-browserpass-git/src/go/src/github.com/gokyle/twofactor/totp.go:56 +0x28
github.com/gokyle/twofactor.(*TOTP).OTP(0xc0000c6170, 0x4e, 0x5aa840)
    /tmp/yaourt-tmp/aur-browserpass-git/src/go/src/github.com/gokyle/twofactor/totp.go:37 +0x2b
github.com/dannyvankooten/browserpass.parseTotp(0xc0000e40a0, 0x4e, 0xc00005c600, 0xffb, 0xc0000e40a0)
    /tmp/yaourt-tmp/aur-browserpass-git/src/go/src/github.com/dannyvankooten/browserpass/browserpass.go:215 +0xce
github.com/dannyvankooten/browserpass.parseLogin(0x5a9680, 0xc00000e0b8, 0xc00000e0b8, 0x5a9680, 0xc00000e0b8)
    /tmp/yaourt-tmp/aur-browserpass-git/src/go/src/github.com/dannyvankooten/browserpass/browserpass.go:239 +0x427
github.com/dannyvankooten/browserpass.readLoginGPG(0x5a9680, 0xc00000e090, 0x0, 0x0, 0x0)
    /tmp/yaourt-tmp/aur-browserpass-git/src/go/src/github.com/dannyvankooten/browserpass/browserpass.go:186 +0x1ba
github.com/dannyvankooten/browserpass.Run(0x5a9680, 0xc00000e010, 0x5a96a0, 0xc00000e018, 0x0, 0x0)
    /tmp/yaourt-tmp/aur-browserpass-git/src/go/src/github.com/dannyvankooten/browserpass/browserpass.go:109 +0x775
main.main()
    /tmp/yaourt-tmp/aur-browserpass-git/src/browserpass/cmd/browserpass/main.go:27 +0xc1

maximbaz commented 5 years ago

AUR package browserpass-git doesn't follow the correct build steps, I wouldn't expect it to work. I'm not maintainer of that package at the moment, and it isn't very high on my priority list 😉

browserpass / browserpass-legacy