sseemayer / keepass-rs Goto Github PK

Binary attachments can be provided both in the inner header of a KDBX4 file, and also as a <Binaries> tag inside of the <Meta> tag (this probably is only used in KDBX version 3). Both are currently handled with the BinaryAttachments and BinaryAttachment types, which is not a clean way to handle the data.

Split into separate types for attachments that are provided in the header and in the XML part of the file.

I'm seeing a bunch of warnings from our workflows, and the action we are using is unmaintained. I suggest we migrate to rust-toolchain, which is actively maintained.

This was mentioned here.
See also the comment here.
I think this is the only field that we observed in the KeePassXC database dump that is not implemented yet.

When having the same title twice in a group, only one will show up in the groups iter(). The other group or entry won't show up at all. It does not seem to be accessible through other means as well. Is there something I'm missing?

Additional context
I was trying to create a reproducible test for #16 - looks like I can't create an empty title (only Some("")). Anyways, trying to come up with such a test unearthed this issue.

Something similar to what is done in KeePassXC, which can optionally use the HMAC-SHA1 challenge response to unlock the database.
I found the yubikey.rs library, but I'm not sure that it supports HMAC-SHA1 yet. Also, I tried to compile it real quick and I think it requires some C libraries. Ideally we would use a pure Rust implementation, but I'm not sure if there is one available.

warning: 1 warning found

Crate: quote
Version: 1.0.2
Warning: package has been yanked!
Dependency tree:
quote 1.0.2
___ syn 1.0.5
_ ___ proc-macro-hack 0.5.11
_ ___ hex-literal-impl 0.2.1
_ _ ___ hex-literal 0.2.1
_ _ ___ keepass 0.4.5
_ ___ hex-literal 0.2.1
___ proc-macro-hack 0.5.11

warning: 1 warning found!

Updating the proc-macro-hack crate with cargo addresses the issue.

Entry::AutoType.sequence is one example of data that is not being populated from KDBX4 files but is working fine with KDBX3 files.

I actually stumbled onto this issue attempting to pull ExpiryTime from the database for #29. My changes in xml_parse.rs seem to be working great with KDBX3 files but showing gibberish for this XML value in KDBX4.

Sadly, I do not yet have the rust skills address this myself. But, I am putting together a draft PR updating entry_tests.rs and test_db_kdbx4_with_password_aes.kdbx to illustrate the Entry::AutoType.sequence behavior.

I've created a keyfile with my KeePassX 2.0.3 client. You can find them as *_file.kdbx and keyfile.key here.

Using my combination with the library only yields Cryptography error and will not open the file.

The main difference I can see is this: Your key file is in binary format, showing binary data, while the one created by my Keypass client is an XML file containing seemingly random data.

As we talked about this in a pull request in the keepass-diff repository, I hope you can have a look into this.

You can use the example files from my repository if you like to create a test case around them.

I ran into an issue decrypting my .kdbx file using keepass-diff and was told to report it here as I believe It's in issue at src/hmac_block_stream.rs:21. Full backtrace follows:

$ RUST_BACKTRACE=1 cargo run -- file1.kdbx file2.kdbx --same-password
Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/keepass-diff /home/susurrus/Personal/OneRing.kdbx /home/susurrus/Personal/OneRing.sync-conflict-20210404-004755-OP4M5NG.kdbx --same-password`
Password for both files: 
thread 'main' panicked at 'range end index 1398351512 out of range for slice of length 237116', /home/susurrus/.cargo/registry/src/github.com-1ecc6299db9ec823/keepass-0.4.7/src/hmac_block_stream.rs:21:22
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
   1: core::panicking::panic_fmt
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
   2: core::slice::index::slice_end_index_len_fail
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/slice/index.rs:41:5
   3: <core::ops::range::Range<usize> as core::slice::index::SliceIndex<[T]>>::index
             at /home/susurrus/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:238:13
   4: core::slice::index::<impl core::ops::index::Index<I> for [T]>::index
             at /home/susurrus/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:15:9
   5: keepass::hmac_block_stream::read_hmac_block_stream
             at /home/susurrus/.cargo/registry/src/github.com-1ecc6299db9ec823/keepass-0.4.7/src/hmac_block_stream.rs:21:22
   6: keepass::parse::kdbx4::parse
             at /home/susurrus/.cargo/registry/src/github.com-1ecc6299db9ec823/keepass-0.4.7/src/parse/kdbx4.rs:260:9
   7: keepass::db::Database::open
             at /home/susurrus/.cargo/registry/src/github.com-1ecc6299db9ec823/keepass-0.4.7/src/db.rs:262:17
   8: keepass_diff::kdbx_to_group::{{closure}}
             at ./src/main.rs:189:22
   9: core::result::Result<T,E>::and_then
             at /home/susurrus/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:708:22
  10: keepass_diff::kdbx_to_group
             at ./src/main.rs:186:5
  11: keepass_diff::main
             at ./src/main.rs:152:24
  12: core::ops::function::FnOnce::call_once
             at /home/susurrus/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I didn't see any binary within this library to assist with testing and verifying keepass files, but KeePassXC 2.6.4 can open it successfully, so I assume that there is an issue in this library. I can do some Rust coding, so can contribute here if it'll help, though I don't know this library or the keepass format very well. Any suggestions for where to start to try to debug this? Given the size of this value, I'm wondering if there's a byte-order issue with the indexing here.

Originally reported in Narigo/keepass-diff#32.

Related to #33 and #29

In many recent issues, work on the underlying XML file that becomes available after decryption is needed, so it would be nice to have a tool for dumping the XML file inside of KDBX databases for easier development and debugging.

I was just wondering if there's any way to use this library to decrypt KeePass databases that use both a password and key file, as opposed just a master password.

I'm guessing that would require changes to this crate, but I thought I'd ask if there's some easy method, like combining the master password and text of the keyfile somehow

The order is lost on groups when iterating.

Disclaimer: I've not looked at the internals of this crate nor the keepass format.

Assuming this crate reads everything in order from the .kdbx file, a BTreeMap would retain said order. If not, perhaps there is an order index somewhere on each group which could help?

When you provide a defect kdbx file (in detail: a kdbx file with a size less than 12 byte) to Database::open(), it will panic in DatabaseVersion::parse() due to slice operations without a previous length check.

Happened accidently to me, since I provided the same File object twice to Database::open(), being at EOF position the second time (due to the source.read_to_end() call in Database::open()).

This is similar to #171. I'm considering implementing Zeroize on the Protected variant of Value. I'm not sure if it's possible to implement it only on one of the variants, but worst case we could implement Zeroize on all of Value.

That being said, I'm not sure this task is even required since Value::Protected uses SecStr.

We are currently keeping UUIDs from the XML document as Base64-encoded strings, but could parse them into proper Uuid objects. Also, UUIDs generated by us might not be base64 encoded, leading to incorrect XML output.

Really nice to have this useful lib in rust. However, it seems there's no way to save to a keepass file at the moment. Does it plan to add so?

Do you know if there's any way to access the custom AutoType sequence of an entry?

I'm referring to the string of text "{USERNAME}{DELAY 1000}{TAB}{PASSWORD}{ENTER}" in the screenshot below:

I've been trying things like e.get("AutoType"), e.get("AutoTypeSequence") and e.get("KeystrokeSequence") but no dice apparently.

I also searched the KeePassXC repo for "sequence" for more ideas both only got errors.

Any ideas?

(and possibly kdbx3/2/..)
Hi
I would like to be able to identify the trash bin (/recycle bin) group, I have a PR in progress that could use some.. heavy guidance on how to implement it best, but it is "working" for my project, on my usecase.

I'll submit it shortly

Apparently the URL to fetch the code coverage SVG badge now needs a token passed a query parameter. I don't have access to the settings page in codecov, so I can't generate the new URL myself. Here is the documentation for codecov status badges.

Hi!

I am using this crate for my app and it works great on amd64 and aarch64 but on armv7 (32bit) it gives me KDBX error: Incorrect key specified.

This is for databases with password-only and key+password

Also happens when running kp-dump-xml on armv7:

david@odroidxu4:~/keepass-rs$ cargo run --release --features "utilities" --bin kp-dump-xml -- test.kdbx 

Tried on the 0.4.9 crate and on master.

Let me know if I can give you some more information

I am building natively on armv7-unknown-linux-gnueabihf

I am planning to use this crate for a small personal project not only to familiarize myself with Rust but also to use as a [hopefully] short term solution to this KeePassXC feature request that has been around in one form or another for several years.
keepassxreboot/keepassxc#4624

In my limited testing so far, I have found this crate very easy to use and robust. However, it does not provide access to the Group or Entry expiration information which I will need for my project. If there is no interest in merging something like that into this crate, I will only do enough work to fulfill my own needs. However, I think this might be useful to others; so, I am more than willing to fully build this feature out for inclusion here.

My initial plan is to loosely model a new Expires struct and associated population on the existing AutoType struct.

Additionally, I will probably also want to add an Exclude from database reports indicator on Entry. For this, I am thinking a new bool in Entry would suffice.

I welcome (and will appreciate) any feedback on both the likely inclusion of these features into this crate as well as my planned approach.

Thank you.

Hi @sseemayer , I want to be member of this project. do you approve?

It would be nice if OpenDBError implemented std::error::Error.

In order for OpenDBError to implement Error, the following needs to be decided:

• Error requires its implementers to implement Debug and Display. OpenDBError already implements Debug, but it would also have to be able to display itself in a pretty way. I don't know enough about the domain to know if there is a simple "nice" string representation for the types of errors.
• Error requires a .description(&self) -> &'static str function. It's probably enough to have a simple static string for each variant, and return that. (see what std::error::Error does)

entries with empty titles panic the parser.

I don't think we currently document the minimum version of Rust supported by the library. There is now a Cargo field that we can use to document this requirement. There's also a crate that can allow us to find the current minimum version we support. I'm thinking we could eventually use this crate to detect in CI if the MSRV changes. This will allow us to make an explicit decision when bumping the minimum version.

The TOTP struct holds the totp secret, so it would make sense to zeroize it when freed.

Support for de/serialization would be much appreciated.

Just noticed that I get an Encountered an invalid compression suite error when I try to unlock a KeePass database that uses the "Argon2" key derivation function or the "AES-KDF (KDBX 4)" key derivation function. As of version 2.3.0, KeePassXC defaults to KDBX 4.0 and uses the Argon2 KDF.

I'm not sure if this issue with this crate is that it can't open KDBX 4 files or whether it can't do the Argon2 or AES KDF for KDBX 4.0 databases.

It'd be super cool if this crate could handle all these cases!

I want to use this tool, without installing RUST.
There is a command to create the docker image, which raises these two questions:

• Why don't you provide the image?
• The build command expects Containerfile.install. But there is no link to it. Using RUST we can get it, but then we won't need it.

In the last line of that page it says "please reach out!" but there is no link for how to reach out.

@pludi reported this in keepass-diff.

I have done some additional debugging with the provided files. The transformation_rounds when parsing the header is set to32804232 and thus the encryption for-loop runs very often. Since my Keepass client (not keepass-diff) can open these files pretty quickly, I wonder how that really works...

Additional context

• Test1.kdbx.gz, Password: Test1
• Test2.kdbx.gz, Password: Test2
• Initial issue report by @pludi over here: Narigo/keepass-diff#24

It seems like there were some major updates to the aes package and the currently used 0.3.2 version even got a cargo yank.

I haven't looked into what changes would be necessary, I only got the heads-up thanks to people failing to install keepass-diff#34 without the --locked flag.

error: Vulnerable crates found!

ID: RUSTSEC-2019-0011
Crate: memoffset
Version: 0.2.1
Date: 2019-07-16
URL: https://rustsec.org/advisories/RUSTSEC-2019-0011
Title: Flaw in offset_of and span_of causes SIGILL, drops uninitialized memory of arbitrary type on panic in client code
Solution: upgrade to >= 0.5.0
Dependency tree:
memoffset 0.2.1

error: 1 vulnerability found!

Because of the old version of rust-argon2, which pulls an old version of crossbeam, which pulls an old version of crossbeam-epoch, which pulls a vulnerable version of memoffset. Updating the rust-argon2 version in cargo.toml and updating it to update the cargo.lock addresses the issue.

error: Vulnerable crates found!

ID: RUSTSEC-2019-0029
Crate: chacha20
Version: 0.2.1
Date: 2019-10-22
URL: https://rustsec.org/advisories/RUSTSEC-2019-0029
Title: ChaCha20 counter overflow can expose repetitions in the keystream
Solution: upgrade to >= 0.2.3
Dependency tree:
chacha20 0.2.1
___ keepass 0.4.5

warning: 2 warnings found

Crate: chacha20
Version: 0.2.1
Warning: package has been yanked!

Crate: quote
Version: 1.0.2
Warning: package has been yanked!
Dependency tree:
quote 1.0.2
___ syn 1.0.5
_ ___ proc-macro-hack 0.5.11
_ ___ hex-literal-impl 0.2.1
_ _ ___ hex-literal 0.2.1
_ _ ___ keepass 0.4.5
_ ___ hex-literal 0.2.1
___ proc-macro-hack 0.5.11

error: 1 vulnerability found!
warning: 2 warnings found!

Updating the chacha20 version in cargo.toml and updating it to update the cargo.lock addresses the issue.

The 0.6.0 version of the crate will bring with it the new DatabaseKey type which has to be passed to Database::open and Database::save, e.g. like this:

Database::open(
    &mut db_file,
    DatabaseKey::with_password_and_keyfile("demopass", &mut File::open("keyfile.xml")?)
)

similarly, there are also DatabaseKey::with_password and DatabaseKey::with_keyfile. Also, we currently allow the DatabaseKey to be constructed directly:

DatabaseKey {
    password: Some("demopass".to_string()),
    keyfile: Some(&mut File::open("keyfile.xml")?),
}

While this new design brings some code reuse and readability improvements, there are also disadvantages with it:

• The DatabaseKey still gets consumed when passing it to one of the Database::* methods, and we cannot implement Clone on it since it contains a handle to Read for the keyfile.
• Users of the keepass-rs crate might not know at compile time with which combinations of key elements a database will be opened, so they will need the Option-based parameters. We currently make the internal attributes of the DatabaseKey public, but that prevents us from ever changing the interface in the future.

Since this part of the crate feels "not quite there yet", I would like to solicit some additional inputs about alternate designs.

I want to add a GHA workflow that automates releases to crates.io. I propose using a variant of semantic-release, which uses conventional commits to determine the next release number and generate the release notes. I would also add a CI step that validates the format of the commit message, which would make the git history more consistent and helpful. @sseemayer let me know what you think.

If you implement From<std::io::Error>, From<keypass::decompress::DecompressionError> and From<crypto::symmetriccipher::SymmetricCipherError> for OpenDbError, you can eliminate the need to call .map_err() on any io errors when using try! here, here, here, and here

GitHub supports creating a SECURITY.md file that will be visible to contributors when they create a PR. See here for details. I think this is relevant for this project in order to allow for responsible disclosure of security vulnerabilities.

We already have Argon2d and Argon2id with #162. It was proposed here to add support for Argon2i as well.

Description
Files with larger attachments (2MB+) seem to be problematic when opened.

Here is a larger test file attached (needs unzip since GitHub doesn't allow attaching kdbx directly): large-withfile.kdbx.zip
The password is samplepassword.

The reproducer file provided was provided by @Petemir, who created the original report in keepass-diff.

Additional debug
I have tried opening the attached file on the latest master with v0.4.9. Both debug commands fail with "corrupt deflate stream":

cargo run --features="utilities" --bin kp-dump-xml large-withfile.kdbx
Password (or blank for none): 
Error: IO { e: Custom { kind: InvalidInput, error: "corrupt deflate stream" } }

cargo run --features="utilities" --bin kp-show-db large-withfile.kdbx
Password (or blank for none): 
Error: IO { e: Custom { kind: InvalidInput, error: "corrupt deflate stream" } }

Adding debug println! statements, I could see that it fails decompressing block_buffer_compressed in kdbx3 parse.

In attempting to my keepass-rs dependency in Medic from keepass-rs 0.4.1 to 0.4.4, I discovered a quirky bug.

A test in which I attempt to unlock a KeePass database that's version 3.1 (rather than the newer 4.0) and does not require a keyfile seemingly results in an infinite loop. Here's the test, and here's the test database. I believe this test would pass if I were using keepass-rs 0.4.1, but thanks to some strange build situation, I've found it difficult to revert back to 0.4.1.

Strangely, Medic (using keepass-rs 0.4.4) can successfully unlock a 3.1 database if it requires a keyfile to be decrypted.

This could totally be an issue with my code -- I just did a large refactoring to handle errors better.

I'm getting a bunch of warnings when merging databases with keepassxc-cli merge after using the update_history function.
The warnings look like this:

Entry of entry_title[25ba7e55577841ba8f3ca23a96d61bc4] contains conflicting changes - conflict resolution may lose data!

The warning is raised here. I think the problems stems from the way the entries are compared here

Thanks again for fixing the bug I ran into. :-)

I'd further more like to use a database stored on an S3 bucket, using crate s3reader right away, which implements the read trait. Therefore it would be really helpful, if Database::open() would support specifying a class implementing the std::io::Read trait, as such (or similar):

pub fn open<R: std::io::Read>(
source: &mut dyn R,
key: DatabaseKey,
) -> Result<Database, DatabaseOpenError> {
...
}

I didn't compile and test it, but I hope you got my point. :-)

The project description for the GitHub project is still using the previous description. We could update it with the new project description, as found in the README.

This crate is used in keepass-diff , I'm trying to check diffs with my two out of sync .kdbx(kdbx4) its about 9-11Mb , but it always shows incorrect key specified. Built from aur https://aur.archlinux.org/packages/keepass-diff

This is the full backtrace.

thread 'main' panicked at 'Error opening database A: IncorrectKey', src/main.rs:117:18
stack backtrace:
0:     0x55b96a052cbd - <unknown>
1:     0x55b96a07013c - <unknown>
2:     0x55b96a050931 - <unknown>
3:     0x55b96a0544b5 - <unknown>
4:     0x55b96a0541d1 - <unknown>
5:     0x55b96a054a46 - <unknown>
6:     0x55b96a054937 - <unknown>
7:     0x55b96a0531b4 - <unknown>
8:     0x55b96a054669 - <unknown>
9:     0x55b969f7fd73 - <unknown>
10:     0x55b969f7fe03 - <unknown>
11:     0x55b969f926d0 - <unknown>
12:     0x55b969f87976 - <unknown>
13:     0x55b969f87f95 - <unknown>
14:     0x55b96a04be6e - <unknown>
15:     0x55b969f96082 - <unknown>
16:     0x7fda3e5e9290 - <unknown>
17:     0x7fda3e5e934a - __libc_start_main
18:     0x55b969f7ffb5 - <unknown>
19:                0x0 - <unknown>

What would you think of bumping the edition for this from its current, default, 2015 to the latest 2018?

This will require a few, hopefully minor, source code changes which I am willing (and I believe able) to tackle after completing #29, assuming you think this is worthwhile.

To make contributing a bit easier with consistent style

See #118 (comment) for details

As suggested here, implementing Zeroize would increase the security of the key data in memory.

Here is what I expected an entry to look like. Imo it's better.

Entry {
    pub title: String,
    pub username: String,
    pub password: String,
    // ...
}

My expectation comes seeing it this way in a GUI. Another reason is that renaming a string key is not a good idea.

This would require adding custom serde code tho..

This CI job was added after we released 0.6 and realized that docs.rs was not able to build the docs. I think it should be required to make sure that changes don't break the docs.

cc @sseemayer