Add Ability to Manage New & Existing Passwords

[patgmiller]

This will extend functionality of the browserpass-extension to save, edit, and remove new and existing passwords for each of a user's configured password stores.

[erayd]

I haven't had the time to look at it properly yet, but skimmed it and noticed it seems to be writing decrypted contents to disk. We should never, ever be doing that - it's totally unacceptable to be bypassing the user's storage encryption (especially without their consent!), and on many platforms will result in leaving plaintext secrets somewhere from which they are recoverable.

I will never be OK with any solution which saves plaintext to the filesystem.

Shall we consider perhaps unconditionally trim the trailing whitespaces in the login entry contents, and leave it at that?

I would prefer that we actually fix the problem properly. Unconditional trim would be fine for read-only stuff, but given that we're going to be editing & saving content, I think it's important that we don't unliaterally introduce changes to it. Newline changes in particular (either appearing or disappearing) can mess with people's scripted workflows.

[maximbaz]

You are completely right, I agree. OK I tried to reproduce now, to confirm that the issue is on Go side, but it seems just fine for me? @patgmiller could you try this experiment too?

Snippet for copy-paste

$ printf nonewline > nonewline

$ printf 'withnewline\n' > withnewline

$ cat nonewline
nonewline
$ cat withnewline
withnewline

$ gpg --encrypt -r 0xB5DB77409B11B601 nonewline

$ gpg --encrypt -r 0xB5DB77409B11B601 withnewline

$ c main.go
package main

import (
    "bytes"
    "fmt"
    "os"
    "os/exec"
)

func GpgDecryptFile(filePath string) string {
    passwordFile, err := os.Open(filePath)
    if err != nil {
        return ""
    }

    var stdout, stderr bytes.Buffer
    gpgOptions := []string{"--decrypt", "--yes", "--quiet", "--batch", "-"}

    cmd := exec.Command("gpg", gpgOptions...)
    cmd.Stdin = passwordFile
    cmd.Stdout = &stdout
    cmd.Stderr = &stderr

    if err := cmd.Run(); err != nil {
        return ""
    }

    return stdout.String()
}

func main() {
    fmt.Printf("-->%v<--\n", GpgDecryptFile("./nonewline.gpg"))
    fmt.Printf("-->%v<--\n", GpgDecryptFile("./withnewline.gpg"))
}

$ go run ./main.go
-->nonewline<--
-->withnewline
<--

[patgmiller]

@maximbaz @erayd it wasn't happening at all with a trailing newline, it was happening when there was extra white space at the end of the last line which was not the new line character. It was just adding a newline.

That said, I can't duplicate it with the simplified commands, so there is still an element that the tests are missing. With this we've eliminated everything but Native Host interaction with back end code. I've attached an actual bash script which does what you provided (below), you'll have to rename it after downloading. Here's screen grab of my run.

In the native host code changes, I've replaced the plain text file test approach i did previously, with a pipe output and pushed it to the native branch. It

1. fixes the problem
2. doesn't modify any user content. either for code in host or extension
3. doesn't write plain text to storage
4. can hardly be called over engineering

whitespace_test.sh.txt

[maximbaz]

it wasn't happening at all with a trailing newline, it was happening when there was extra white space at the end of the last line which was not the new line character. It was just adding a newline.

Right, sorry, you mentioned that before and it slipped my attention...

That said, I can't duplicate it with the simplified commands

Yeah, I am trying your script and I can't replicate the issue with it either. To be honest, I can't replicate the issue even in the browser... With the latest code from this PR, and native host from this PR. Could I be doing something wrong? Or could it be something else at play here, like CRLF vs LF for example? Do you have an actual decrypted file which reproduces the problem? If so, could you zip it and attach, so I could try on it?

Screenshots

[patgmiller]

@maximbaz you could be right about there being something more at play, because I tried checking out the helpers/helpers.go version prior to my changes and it wasn't happening exactly as before. Give me a couple days to see if I can still reproduce it or not.

[patgmiller]

@maximbaz I am able to replicate it now.

With the browserpass-extension at the latest for this PR. In your browserpass-native repo do the following:

1. checkout the HEAD of my implement-tree-save-delete branch from my repo [email protected]:patgmiller/browserpass-native.git
2. then git checkout a0672a21fabbca1751153aa7b9ee1929d66e1146 -- helpers/helpers.go this was the first commit i added to your original changes in which I only changed the version.
3. build native host and copy/move the browserpass binary to the correct location if different

In replicating the issue I am getting an extra new line every time, but you have to do WITH the browserpass-native and broswerpass-extension. As I mentioned originally I believe it is a combination of the native host app with the native messaging of the browser.

Afterwards either do a hard reset, git reset --hard, or git checkout -- helpers/helpers.go , rebuild and re-test.

[maximbaz]

I've just tried again, but weirdly I'm still not able to reproduce it 😐

I hope I didn't misunderstand some of the repro steps, so let me try to very carefully document everything I did:

1. In browserpass-native, select the commit with the new version only a0672a2, confirm there are no local changes:

$ git status
HEAD detached at a0672a2
nothing to commit, working tree clean

2. Build using make, then move the binary to /usr/bin/browserpass (in my case it's the location).

3. In browserpass-extension, make sure I'm on the latest commit in your PR:

$ git status
On branch edit-ui
Your branch is up to date with 'patgmiller/edit-ui'.

nothing to commit, working tree clean

$ git rev-parse HEAD
997aaccc49f22b67c997408130237019dd809f5d

4. Then also make and refresh it in the Chromium browser in chrome://extensions/.

5. In the settings of Browserpass, select password store that contains the nonewline.gpg and withnewline.gpg that were created using your script above.

6. Now in Chromium, click on the popup, pick any of the two, and click the Edit button, and I'm expecting to see the extra newline, is that correct?

Could it be a browser or OS combination, or specific password entry? Could you perhaps encrypt the test credentials file, on which you can reproduce the issue, with your and my PGP keys, so that we have an identical file, that we both can decrypt and use for testing? My public key is here.

So to summarize, I've never been able to reproduce it so far in any conditions 😞 @erayd what about you, are you able to reproduce?

Is there anything else in this PR, that is pending before merge, besides this issue?

[patgmiller]

@maximbaz if we're being exact there was one minor step omitted, which was to checkout just the helpers/helpers.go version of the that commit in the browserpass-native repo.

git checkout a0672a21fabbca1751153aa7b9ee1929d66e1146 -- helpers/helpers.go

But for all intensive purposes your steps and test should've worked to reproduce the issue. So the only thing left I can think is perhaps it is a version of Chrome. However, my latest commit to the PR in browserpass-native fixes it. As far as I know the pseudo extra line was the only remaining issue to resolve.

For clarity I am on the following setup:

Google Chrome Version 108.0.5359.71 (Official Build) (64-bit)

lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.6 LTS
Release:	18.04
Codename:	bionic

uname -a
Linux friday 4.13.0-41-generic #46~16.04.1-Ubuntu SMP Thu May 3 10:06:43 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

[patgmiller]

@maximbaz for kicks here is a file i was able to reproduce the issue with
example-extra-lines.gpg.txt drop the .txt ending and then decrypt

To be clear, it has first line password and followed by two blank lines, shown below, for a total of 3 lines. But following my earlier test, will have 4 lines in the browser plugin.

Here is example content

iekideizaCee1xohjooGeir0

And here are non plugin output

╭─◦ /tmp  ‹kube: › ‹vendor: ›
╰─◦ devpass browserpass/google.com/example-extra-lines 
iekideizaCee1xohjooGeir0


╭─◦ /tmp  ‹kube: › ‹vendor: ›
╰─◦ devpass browserpass/google.com/example-extra-lines | wc -l 
3

Also for clarity devpass is just an alias using pass

╭─◦ /tmp  ‹kube: › ‹vendor: ›
╰─◦ which devpass
devpass () {
	PASSWORD_STORE_DIR=$HOME/path/to/the/dir/.password-store pass $@
}

Attached is short video of lines in plugin. I hope that helps, but it is not a problem in the latest version of the branch for the PR browserpass-native.

bp-plugin-extra-line.webm

[erayd]

So to summarize, I've never been able to reproduce it so far in any conditions 😞 @erayd what about you, are you able to reproduce?

I could certainly reliably reproduce it before. I'm away from my usual computer until after New Year, but will take a look then and see if I can still reproduce it.

Is there anything else in this PR, that is pending before merge, besides this issue?

I think it's just this. Will need a rebase on master though.

[maximbaz]

Thank you for the file @patgmiller! I believe it helped me to finally understand what's going on, and I hope now to clarify it for you as well. Long post ahead, brace yourself 😁

Everything starts with agreeing on a definition of a "line".

POSIX standard defines a line as such:

A sequence of zero or more non-newline characters plus a terminating newline character.

wc -l counts therefore the number of newline characters (also known as \n, or 0a in hex).

To demonstrate this, make sure to use printf and not echo, which gives precise control over every character to be printed.

$ printf 'first line' | wc -l
0

The output of printf produces an "invalid" line according to POSIX standard, a line that does not end with a newline character, therefore wc -l tells us that there are zero lines.

The snippet below is just to demonstrate that neither gpg nor pass are important here:

$ printf 'first line' > /tmp/entry
$ gpg --encrypt -r $id /tmp/entry
$ gpg --decrypt /tmp/entry.gpg | wc -l
0
$ PASSWORD_STORE_DIR=/tmp pass entry | wc -l
0

It really is just the number of newline characters that wc -l counts.

Now, if you explicitly add a newline character at the end, then you'd get the expected result of 1:

$ printf 'first line\n' | wc -l
1

Another way to see this is to explore file in hex, where 0a would represent the "hidden" newline character:

$ printf 'first line' | od -A n -t x1z -v
 66 69 72 73 74 20 6c 69 6e 65                    >first line<

$ printf 'first line\n' | od -A n -t x1z -v
 66 69 72 73 74 20 6c 69 6e 65 0a                 >first line.<

Notice that we can clearly see the 0a newline character in the end of the second output.

---

Now, let's take your example-extra-lines.gpg.txt file and analyze its contents with hex:

$ gpg --decrypt example-extra-lines.gpg 2>/dev/null | od -A n -t x1z -v
 69 65 6b 69 64 65 69 7a 61 43 65 65 31 78 6f 68  >iekideizaCee1xoh<
 6a 6f 6f 47 65 69 72 30 0a 0a 0a                 >jooGeir0...<

You can clearly see that the file ends with three newline characters, three 0a.

Let's look at two browserpass-native hosts, first the version from my branch + version change (i.e. commit a0672a2):

As you can see, the file contents that the extension receives from native hosts correctly contains three \n newline characters, getting the entry contents exactly as it is stored on disk.

If we view the entry (not in the edit mode yet), by selecting everything, we see three lines of text (one line of text "iekideizaCee1xohjooGeir0" and two empty lines - probably what you also see in your own editor:

It is in the edit mode, in the textarea, where cursor is seemingly on the fourth line:

So it is an issue with Chromium, that it decides to represent each of the three newline characters on a new line.

However, if I don't change anything, press save, and inspect the contents of the file again, I will correctly see three newline characters - so the file contents correctly didn't change, and that's what's important.

$ gpg --decrypt example-extra-lines.gpg 2>/dev/null | od -A n -t x1z -v
 69 65 6b 69 64 65 69 7a 61 43 65 65 31 78 6f 68  >iekideizaCee1xoh<
 6a 6f 6f 47 65 69 72 30 0a 0a 0a                 >jooGeir0...<

Now, let's repeat the experiment with the latest commit from your branch in browserpass-native (i.e. 4c22ee04).

If I explore the contents of the file, as received from native host, it would miss one newline character:

In the edit part of the extension, I will see only three lines, as we would expect:

However, if I don't change anything, click Save and explore the file contents, we observe that we incorrectly modified the file, and removed one line:

$ gpg --decrypt example-extra-lines.gpg 2>/dev/null | od -A n -t x1z -v
 69 65 6b 69 64 65 69 7a 61 43 65 65 31 78 6f 68  >iekideizaCee1xoh<
 6a 6f 6f 47 65 69 72 30 0a 0a                    >jooGeir0..<

Furthermore, if I repeat the action again (popup -> edit -> make no changes -> press Save -> explore file contents), I will see that the file is again modified:

$ gpg --decrypt example-extra-lines.gpg 2>/dev/null | od -A n -t x1z -v
 69 65 6b 69 64 65 69 7a 61 43 65 65 31 78 6f 68  >iekideizaCee1xoh<
 6a 6f 6f 47 65 69 72 30 0a                       >jooGeir0.<

And then again:

$ gpg --decrypt example-extra-lines.gpg 2>/dev/null | od -A n -t x1z -v
 69 65 6b 69 64 65 69 7a 61 43 65 65 31 78 6f 68  >iekideizaCee1xoh<
 6a 6f 6f 47 65 69 72 30                          >jooGeir0<

Until no more newline characters are left, and the file becomes stable. At this point, wc -l would (correctly) identify as the file containing zero lines.

$ gpg --decrypt example-extra-lines.gpg 2>/dev/null | wc -l
0

---

To summarize:

• I believe the changes in the browserpass-native should not be made, and we should return to commit a0672a2.
• We can decide whether we want to do some magic to "fix" Chromium's representation of content in textarea on javascript side (e.g. by removing the last newline character before showing the edit form, and then adding it back before Save is pressed) or doing nothing. Because we don't know if all browsers of all versions are affected, as well as we probably won't be actively monitoring when Chromium gets this fixed, my personal vote is for not doing anything, but I don't have strong opinion here.
• I care for us not changing file contents on disk if user made zero changes in the textarea, and I care for the native host to correctly send all characters of the file.

[patgmiller]

@maximbaz technically echo -n "" would also not include the extra new line, but printf is more explicit so I'm good with either. thanks so much for your additional work on it. Personally I agree with your suggestions and I would also vote we do nothing.

[maximbaz]

Cool! We may await @erayd's comment as well, but otherwise if you have a chance to rebase on master, please go ahead, because from my personal view this is ready to be merged once rebase conflicts are resolved! 🚀

[patgmiller]

@maximbaz if the user hasn't made any changes, we can probably see that b/c a sha sum is generated for existing contents before presenting the edit and then just skip saving when no actual changes have been made.

[patgmiller]

Pending @erayd comments, I will also revert additional changes to browserpass-native if that is the agreed decision.

[maximbaz]

True, or you can just compare the strings directly, it would probably be an overkill to compute hashsum of what's likely just a few lines of text, instead of just comparing these texts directly. Good idea to do (maybe we can deactivate "Save" button if there are no changes made)

[erayd]

Thank you very much @maximbaz for that detailed analysis, and @patgmiller for your work getting to the bottom of this also. I'm in agreement with you guys - now that it's understood, and it's confirmed it's not changing the file content, I'm happy for us to proceed.

True, or you can just compare the strings directly, it would probably be an overkill to compute hashsum of what's likely just a few lines of text, instead of just comparing these texts directly. Good idea to do (maybe we can deactivate "Save" button if there are no changes made)

I agree with both of those points.

[erayd]

@maximbaz Are we still waiting on anything for this one?

[maximbaz]

I don't think so! I just tested once more in Firefox, everything seems to be great (found a small issue - deleting an entry seems to unnecessarily want to decrypt something - but we can take that separately).

I give my approval 👍 Please go ahead and merge if you are also happy with it!

I'll check the native host side of things, do a code review and merge if I'm happy with everything.

Then maybe we can have a look at what else should go in the next release (at least, perhaps, updating all dependencies?), and coordinate to make a new release? What do you think?

[maximbaz]

I went through the native host code, merged all pending items, fixed some low-hanging fruits from other open issues, updated dependencies, and cut a new release. On its own the new release neither does any harm as it's backwards-compatible, nor does it bring much benefits, but I imagine people will be quite eager to see this one merged and having a release of the extension soon 🙂

[gotjoshua]

any ETA on this one?? super eager!

[erayd]

Thank you very, very much @patgmiller (and apologies for how long it's taken to get this merged). Your significant effort in implementing this is greatly appreciated!

[patgmiller]

@erayd @maximbaz and all, you're welcome! it was tough at times, but hopefully the results are well worth it.