jorabin / KeePassJava2

Java API for KeePass Password Databases - Read/Write 2.x (File versions 3 and 4), Read 1.x
Apache License 2.0
250 stars 71 forks source link

#41 Added support for hash check in the KeyFile (v. 2) #56

Closed giusvale-dev closed 1 year ago

giusvale-dev commented 1 year ago

Hi Jo, Here the hasing check patch.

I added 2 tests on the project (and another db file) to test this task!

I will try to develop the other feature (on this issue) when possible!

Giuseppe

giusvale-dev commented 1 year ago

Update! The other features related to this issue are in progress!

jorabin commented 1 year ago

sorry taken me so long to review

giusvale-dev commented 1 year ago

Hi Jo, Here is an update of the code (no final solution, this is under the test and refactoring again): I have 2 main problems:

This is the current (no definitive) code:

    public static byte[] load(InputStream inputStream) {

        try {

            PushbackInputStream pis = new PushbackInputStream(inputStream);
            byte[] buffer = new byte[65];
            int bytesRead = pis.read(buffer, 0, 65);
            if(bytesRead == 32) {
                return Arrays.copyOf(buffer, bytesRead);
            } else if(bytesRead == 64) {
                byte[] keyFile = Hex.decodeHex(new String(Arrays.copyOf(buffer, bytesRead)));
                return keyFile;
            } else {
                boolean isXml = true;

                try {
                    DocumentBuilder documentBuilder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
                    Document doc = documentBuilder.parse(new PushbackInputStream(inputStream));
                    String version = (String) xpath.evaluate("//KeyFile/Meta/Version/text()", doc, XPathConstants.STRING);
                    String data = (String) xpath.evaluate("//KeyFile/Key/Data/text()", doc, XPathConstants.STRING);
                    if (data == null) {
                        return null;
                    }
                    if (version.equals("2.0")) {
                        byte[] hexData = Hex.decodeHex(data.replaceAll("\\s", ""));
                        MessageDigest md = Encryption.getSha256MessageDigestInstance();
                        byte[] computedHash = md.digest(hexData);
                        String hashToCheck = (String) xpath.evaluate("//KeyFile/Key/Data/@Hash", doc, XPathConstants.STRING);
                        byte[] verifiedHash = Hex.decodeHex(hashToCheck);
                        boolean isHashVerified = Arrays.equals(Arrays.copyOf(computedHash, verifiedHash.length),
                                                    verifiedHash);
                        if (!isHashVerified) {
                            throw new IllegalStateException("Hash mismatch error");
                        }
                        return hexData;
                    }
                    return Base64.decodeBase64(data.getBytes());
                } catch(IOException | SAXException | XPathExpressionException | ParserConfigurationException e) {
                    if(e instanceof SAXException) {
                        isXml = false;
                    } else {
                        throw new IllegalStateException(e.getMessage());
                    }
                }
                if(!isXml) {

                    MessageDigest md = Encryption.getSha256MessageDigestInstance();

                    while(pis.available() != -1) {
                        byte b = (byte) pis.read();
                        md.update(b);
                    }
                    byte[] keyFile = md.digest();
                    return keyFile;
                }
            }

            return null;

        } catch(IOException | DecoderException e) {
            throw new RuntimeException("Key File input stream cannot be null");
        }
  1. In this lane Document doc = documentBuilder.parse(new PushbackInputStream(inputStream)); The parse method closes the inputStream (I tried to pass the pis variable and the result is the same).
  2. I don't like the way to check if the file is an XML.

For that reason, my previous code copied the InputStream in the byte array or another inputStream, because we needed to prevent the input stream from closing.

Do you have any suggestions?

Giuseppe

jorabin commented 1 year ago

Hi Giuseppe

Have a look at the SaxParse example and look at using the sax parser directly. Or maybe use the StAX parser.

If the file is not XML then I guess that SAX parser will throw an exception leaving the stream open) (Guessing that it is is DocumentBuilder that closes it?

The objection to loading the whole thing into DOM is only that if your file happens to be an XML file and it is very large then you could run out of memory. I suppose you could set a small upper limit on how big you think a signature can be and not load it if it's bigger than that. I mean there could be a case where it is a valid file but is very long (because you can have an indefinite number of comments or whitespace) so on the whole using SAX or StAX would be more complete and robust.

Jo

giusvale-dev commented 1 year ago

Hi Jo, I pushed my last version, I hope now is ok!

Give me a feedback!

Have a nice WE! Giuseppe

jorabin commented 1 year ago

Noting that I have updated the file a few times since I left this comment ... will leave alone now.

jorabin commented 1 year ago

hey Giuseppe, how you are well, were you going to continue with this PR?

giusvale-dev commented 1 year ago

hey Giuseppe, how you are well, were you going to continue with this PR?

Dear Jo, I apologize for the late reply it's a busy period!

Unfortunately, I lost the goal of this PR missing your request changes.

However, in my fork repo, I had developed all the modes to make the KeyFile.

So I suggest, aborting this PR then I can create a new PR with all these changes: in this way should simply track the changes.

If you agree, give me feedback and I will make a new PR.

Giuseppe

jorabin commented 1 year ago

Hey Giuseppe. Me too (busy!) and makes sense to start afresh. Did you review my suggestions here would be good if they could be merged with your tests etc?

giusvale-dev commented 1 year ago

Start afresh with new PR