amoseui
commented
1 month ago Closed amoseui closed 1 month ago
amoseui
commented
1 month ago 
minsubb13
commented
1 month ago 해당 이슈로 현재 리뷰 진행 중인 패치들입니다. 참고하세요~
감사합니다! 이것까진 확인해보지 못했는데 확인해보겠습니다
minsubb13
commented
1 month ago /crypto/unexportable_key_win.cc: unexportable_key_win.cc
// line 296
std::array<uint8_t, kSHA256Length> digest = SHA256Hash(data);
// line 330
std::array<uint8_t, kSHA256Length> digest = SHA256Hash(data);SHA256Hash 함수 추적SHA256Hash 함수는 “/crypto/sha2.h”의 다음 함수를 이용하고 있음
// /crypto/sha2.h
CRYPTO_EXPORT std::array<uint8_t, kSHA256Length> SHA256Hash(
base::span<const uint8_t> input);
// /crypto/sha2.cc
std::array<uint8_t, kSHA256Length> SHA256Hash(base::span<const uint8_t> input) {
std::array<uint8_t, kSHA256Length> digest;
::SHA256(input.data(), input.size(), digest.data());
return digest;
}“/crypto/hash.h”와 “/crypto/hash.cc”를 살펴봤을 때 위의 함수와 parameter, return값이 같은 다음 함수를 발견
// /crypto/hash.h
CRYPTO_EXPORT std::array<uint8_t, kSha256Size> Sha256(
base::span<const uint8_t> data);
// /crypto/hash.cc
std::array<uint8_t, kSha256Size> Sha256(base::span<const uint8_t> data) {
std::array<uint8_t, kSha256Size> result;
Hash(HashKind::kSha256, data, result);
return result;
}new API로 migrate하기 편하게 parameter와 return 자료형을 맞춰놓은 것이 아닐까 추측
- #include "crypto/sha2.h"
+ #include "crypto/hash.h"
- std::array<uint8_t, kSHA256Length> digest = SHA256Hash(data);
+ std::array<uint8_t, kSHA256Length> digest = crypto::hash::Sha256(data);
- std::array<uint8_t, kSHA256Length> digest = SHA256Hash(data);
+ std::array<uint8_t, kSHA256Length> digest = crypto::hash::Sha256(data);
이 수정이 맞는지 검증 후 CL 업로드
minsubb13
commented
1 month ago /crypto 디렉토리에서 “sha2.h”의 SHA256Hash 함수를 검색
SHA256Hash 함수를 이용하는 파일 두 개를 발견
본인의 OS가 macOS라 unexportable_key_win.cc 파일은 빌드자체가 안됨. 즉, 수정을 해도 확인할 수 없음 → 문제지점
그래서, unexportable_key_software_unsecure.cc에서 SHA256Hash 함수를 crypto::hash::Sha256 로 변경하였을 때, 적절한 문법 오류(오타같은)를 잡아내며 빌드가 성공적으로 진행되는 것을 확인(?) (unittest빌드를 돌려봤음, 전체 빌드는 돌려보지 않았음)
따라서 unexportable_key_software_unsecure.cc를 다음과 같이 수정
diff --git a/crypto/unexportable_key_software_unsecure.cc b/crypto/unexportable_key_software_unsecure.cc
index d6fd8a4854dae..39090a43edaee 100644
--- a/crypto/unexportable_key_software_unsecure.cc
+++ b/crypto/unexportable_key_software_unsecure.cc
@@ -9,7 +9,8 @@
#include "base/check.h"
#include "build/build_config.h"
-#include "crypto/sha2.h"
+#include "crypto/hash.h"
#include "crypto/signature_verifier.h"
#include "crypto/unexportable_key.h"
#include "third_party/boringssl/src/include/openssl/bn.h"
@@ -64,7 +65,7 @@ class SoftwareECDSA : public UnexportableSigningKey {
std::optional<std::vector<uint8_t>> SignSlowly(
base::span<const uint8_t> data) override {
std::vector<uint8_t> ret(ECDSA_size(key_.get()));
- std::array<uint8_t, kSHA256Length> digest = SHA256Hash(data);
+ std::array<uint8_t, hash::kSha256Size> digest = crypto::hash::Sha256(data);
unsigned int ret_size;
CHECK(ECDSA_sign(0, digest.data(), digest.size(), ret.data(), &ret_size,
key_.get()));
@@ -109,7 +110,7 @@ class SoftwareRSA : public UnexportableSigningKey {
std::optional<std::vector<uint8_t>> SignSlowly(
base::span<const uint8_t> data) override {
std::vector<uint8_t> ret(RSA_size(key_.get()));
- std::array<uint8_t, kSHA256Length> digest = SHA256Hash(data);
+ std::array<uint8_t, hash::kSha256Size> digest = crypto::hash::Sha256(data);
unsigned int ret_size;
CHECK(RSA_sign(NID_sha256, digest.data(), digest.size(), ret.data(),
&ret_size, key_.get()));autoninja -C out/Default crypto_unittests를 이용해 unittest빌드를 진행./out/Default/crypto_unittests --single-process-tests[==========] Running 99 tests from 28 test suites.
[----------] Global test environment set-up.
너무 길어 생략...
[----------] Global test environment tear-down
[==========] 99 tests from 28 test suites ran. (2300 ms total)
[ PASSED ] 99 tests.따라서, unexportable_key_win.cc도 위와 똑같이 적용한다면 문제없지 않을까
현재로서는 unexportable_key_win.cc를 테스트해볼 방법이 없어서,, 이런 경우에는 gerrit에 올려서 CQ dry run으로 확인,,?(야매지만)
amoseui
commented
1 month ago 고생하셨습니다. 패치 올려도 괜찮을 것 같습니다. dry run 은 제가 돌려드릴게요. 일단은 저만 cc로 넣어주시고 dry run 성공 확인 후에 리뷰어 추가로 넣으면 될 것 같습니다.
minsubb13
commented
1 month ago 패치 올렸습니다!
gerrit url: https://crrev.com/c/6518080
amoseui
commented
1 month ago @minsubb13 AUTHORS 에 업데이트가 안되어있어서 에러가 발생했네요.
두가지 중 하나를 선택하면 되는데
2번의 경우 기존 작업하던 브랜치에서 git commit 으로 AUTHORS 내용 추가하고 git cl upload 하시면 업데이트될거에요.
minsubb13
commented
1 month ago @amoseui docs를 뒤로 하고 crypto 패치를 먼저 반영해보는 것은 어떨까 합니다! docs는 리뷰어를 다른 분으로 바꾸거나 하면 금방 반영할 수 있을 것 같아서요! 지금 AUTHORS 내용 추가해서 다시 패치 올렸습니다!
Issue
Bug
Documents
Description
References
(이하 최민섭님 노트 정리)
이슈 요약 : old API를 new API로 변경하는 이슈(?)
/crypto/sha2.h가 old API이고,/crypto/hash.h가 new API인 듯분석
1. sha2.h, hash.h 파일 탐색
/crypto/sha2.h
[source.chromium.org](https://source.chromium.org/chromium/chromium/src/+/main:crypto/sha2.cc)
sha2.h에는 3가지 함수가 선언되어 있음std::array<uint8_t, kSHA256Length> SHA256Hash(base::span<const uint8_t> input) {void SHA256HashString(std::string_view str, void* output, size_t len) {std::string SHA256HashString(std::string_view str) {/crypto/hash.h
[source.chromium.org](https://source.chromium.org/chromium/chromium/src/+/main:crypto/hash.h;l=56;drc=3a3c641c8597f54a4eb6286d816f52b8a649c8bf?q=hash.h&ss=chromium%2Fchromium%2Fsrc)
hash.h에는 hasher라는 class가 선언되어 있고,hash.cc에는 여러 hash 함수가 정의되어 있음2.
/crypto/sha2.h를 import하는 파일 탐색chromium의 src에서 “#include "crypto/sha2.h"”를 검색해봤을 때 많은 파일들에서
#include “crypto/sha2.h”를 찾을 수 있음→ 그 중 하나의 파일을 발견 https://source.chromium.org/chromium/chromium/src/+/main:crypto/unexportable_key_win.cc;l=296;drc=5dcf59b49320519f6ef60cd2a024c125685ed0b0
→
std::array<uint8_t, kSHA256Length> digest = SHA256Hash(data);이 코드를 hash.h가 제공하는 new API로 변경(?)