Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Implement SHA-512 worksheet protection #1171

Open
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
2 participants
@igitur
Copy link
Member

igitur commented Apr 3, 2019

Implements SHA-512 worksheet protection.

From what I gather, it seems there are many more algorithms, hence the verbose XLProtectionAlgorithm enum, but I think Excel defaults to SHA-512.

The default protection is still the old "simple hash" algorithm. I couldn't find a better name for it. We can consider making SHA-512 the default algorithm for new worksheets/workbooks.

A new salt is generated whenever Protect() is called.

Spin count is defaulted to 100 000.

When loading a workbook, the workbook deafult protection algorithm is also deduced from what the current protection methods implemented are. The last used protection algorithm will become a loaded XLWorkbook's default algorithm for new protections.

Still to be implemented is XLWorkbook protection, which is slightly different to XLWorksheet protection.

Fixes #866

@igitur igitur requested a review from Pankraty Apr 3, 2019

@igitur igitur added the enhancement label Apr 3, 2019

@igitur igitur added this to the v0.95 milestone Apr 3, 2019

@igitur igitur force-pushed the igitur:sheet-protection-algorithms branch 3 times, most recently from 392c402 to 7c08d4a Apr 3, 2019

@igitur igitur changed the title Implement SHA-512 worksheet protection [WIP] Implement SHA-512 worksheet protection Apr 3, 2019

@igitur igitur modified the milestones: v0.95, v0.96 Apr 4, 2019

@igitur igitur force-pushed the igitur:sheet-protection-algorithms branch from 7c08d4a to d535e00 Apr 4, 2019

@Pankraty
Copy link
Member

Pankraty left a comment

Sorry it took so long, wanted to make some profiling )

}
});

return System.Convert.ToBase64String(hashedBytes);

This comment has been minimized.

Copy link
@Pankraty

Pankraty Apr 6, 2019

Member

This method can be improved:

            var saltBytes = System.Convert.FromBase64String(salt);
            var passwordBytes = System.Text.Encoding.Unicode.GetBytes(password);
            var bytes = saltBytes.Concat(passwordBytes).ToArray();

            byte[] hashedBytes;
            using (var hash = new SHA512Managed())
            {
                hashedBytes = hash.ComputeHash(bytes);

                bytes = new byte[hashedBytes.Length + sizeof(uint)];
                for (uint i = 0; i < spinCount; i++)
                {
                    var le = BitConverter.GetBytes(i);
                    Array.Copy(hashedBytes, bytes, hashedBytes.Length);
                    Array.Copy(le, 0, bytes, hashedBytes.Length, le.Length);
                    hashedBytes = hash.ComputeHash(bytes);
                }
            }

            return System.Convert.ToBase64String(hashedBytes);

Not only it works several times faster but it also significantly lowers the number of allocations and GC cycles:

Original version
image

Modified version
image

Assert.AreEqual(XLProtectionAlgorithm.SHA512, ws.Protection.Algorithm);
ws.Unprotect("abc");
Assert.IsFalse(ws.Protection.IsProtected);
}

This comment has been minimized.

Copy link
@Pankraty

Pankraty Apr 6, 2019

Member

I would also add a couple of tests for hashing functions just to protect us from accidental modification of algorithms.

E.g.
var hash = CryptographicAlgorithms.GetPasswordHash(XLProtectionAlgorithm.SHA512, "PASSWORDS", "salt", 100000); Assert.AreEqual("kqxXTHzC3J8l5UkuSpKkZOwB9mv4lnTCJeDgpazBNRHkETFHi/VYHMHy7uJhWrBF+h7L96K4rwwUF5lBrXQSWg==", hash);

@igitur

This comment has been minimized.

Copy link
Member Author

igitur commented Apr 8, 2019

Thanks for the feedback. This is still WIP anyway, because I realised we should do the same refactoring on protection for XLWorkbook as we did for XLWorksheet. I should have retracted my request for review, sorry. But the feedback is still valid, so I will incorporate it. I will just re-request a review later when the PR is final.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.