* [bitcoin-dev] The new obfuscation patch & GetStats @ 2015-10-07 17:25 Daniel Kraft 2015-10-07 23:32 ` James O'Beirne 0 siblings, 1 reply; 4+ messages in thread From: Daniel Kraft @ 2015-10-07 17:25 UTC (permalink / raw) To: bitcoin-dev [-- Attachment #1: Type: text/plain, Size: 946 bytes --] Hi! I hope this is not a stupid question, but I thought I'd ask here first instead of opening a Github ticket (in case I'm wrong). With the recently merged "obfuscation" patch, content of the "chainstate" LevelDB is obfuscated by XOR'ing against a random "key". This is handled by CLevelDBWrapper's Read/Write methods, which probably cover most of the usecases. *However*, shouldn't it also be handled when iterating over the database? In particular, I would expect that the obfuscation key is applied before line 119 in txdb.cpp (i. e., while iterating over the coin database in CCoinsViewDB::GetStats). Is there a reason why this need not be done there, or is this an actual oversight? Yours, Daniel -- http://www.domob.eu/ OpenPGP: 1142 850E 6DFF 65BA 63D6 88A8 B249 2AC4 A733 0737 Namecoin: id/domob -> https://nameid.org/?name=domob -- Done: Arc-Bar-Cav-Hea-Kni-Ran-Rog-Sam-Tou-Val-Wiz To go: Mon-Pri [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bitcoin-dev] The new obfuscation patch & GetStats 2015-10-07 17:25 [bitcoin-dev] The new obfuscation patch & GetStats Daniel Kraft @ 2015-10-07 23:32 ` James O'Beirne 2015-10-08 0:29 ` James O'Beirne 0 siblings, 1 reply; 4+ messages in thread From: James O'Beirne @ 2015-10-07 23:32 UTC (permalink / raw) To: Daniel Kraft; +Cc: bitcoin-dev [-- Attachment #1: Type: text/plain, Size: 2141 bytes --] Hey, Daniel. Patch author here. Thanks for the diligence; I think this indeed may be an oversight, though I'm going to need to look into a bit more thoroughly at home. Curious that it didn't fail any of the automated tests. Correct me if I'm wrong, but the only actual invocation of that method is here <https://github.com/bitcoin/bitcoin/blob/master/src/rpcblockchain.cpp#L448> (and even then, proxied through a few layers of CCoinView-machinery). In fact, this line <https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L48> makes me suspect that the implementation of GetStats you reference may be dead code. In any case, you raise a good point: if users of CLevelDBWrapper go directly for the iterator, they run the risk of dealing with obfuscated data. This should be remedied somehow. I'll give it more look this evening. Thanks again for the find, James On Wed, Oct 7, 2015 at 10:25 AM, Daniel Kraft via bitcoin-dev < bitcoin-dev@lists•linuxfoundation.org> wrote: > Hi! > > I hope this is not a stupid question, but I thought I'd ask here first > instead of opening a Github ticket (in case I'm wrong). > > With the recently merged "obfuscation" patch, content of the > "chainstate" LevelDB is obfuscated by XOR'ing against a random "key". > This is handled by CLevelDBWrapper's Read/Write methods, which probably > cover most of the usecases. > > *However*, shouldn't it also be handled when iterating over the > database? In particular, I would expect that the obfuscation key is > applied before line 119 in txdb.cpp (i. e., while iterating over the > coin database in CCoinsViewDB::GetStats). > > Is there a reason why this need not be done there, or is this an actual > oversight? > > Yours, > Daniel > > -- > http://www.domob.eu/ > OpenPGP: 1142 850E 6DFF 65BA 63D6 88A8 B249 2AC4 A733 0737 > Namecoin: id/domob -> https://nameid.org/?name=domob > -- > Done: Arc-Bar-Cav-Hea-Kni-Ran-Rog-Sam-Tou-Val-Wiz > To go: Mon-Pri > > > _______________________________________________ > bitcoin-dev mailing list > bitcoin-dev@lists•linuxfoundation.org > https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev > > [-- Attachment #2: Type: text/html, Size: 3158 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bitcoin-dev] The new obfuscation patch & GetStats 2015-10-07 23:32 ` James O'Beirne @ 2015-10-08 0:29 ` James O'Beirne 2015-10-08 5:14 ` Daniel Kraft 0 siblings, 1 reply; 4+ messages in thread From: James O'Beirne @ 2015-10-08 0:29 UTC (permalink / raw) To: Daniel Kraft; +Cc: bitcoin-dev [-- Attachment #1: Type: text/plain, Size: 2490 bytes --] This has been confirmed as a bug. Thanks again for reporting. I've filed a fix here (https://github.com/bitcoin/bitcoin/pull/6777), and will be writing tests to prevent regressions. On Wed, Oct 7, 2015 at 4:32 PM, James O'Beirne <james.obeirne@gmail•com> wrote: > Hey, Daniel. > > Patch author here. Thanks for the diligence; I think this indeed may be an > oversight, though I'm going to need to look into a bit more thoroughly at > home. Curious that it didn't fail any of the automated tests. > > Correct me if I'm wrong, but the only actual invocation of that method is > here > <https://github.com/bitcoin/bitcoin/blob/master/src/rpcblockchain.cpp#L448> > (and even then, proxied through a few layers of CCoinView-machinery). In > fact, this line > <https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L48> makes > me suspect that the implementation of GetStats you reference may be dead > code. > > In any case, you raise a good point: if users of CLevelDBWrapper go > directly for the iterator, they run the risk of dealing with obfuscated > data. This should be remedied somehow. > > I'll give it more look this evening. > > Thanks again for the find, > James > > On Wed, Oct 7, 2015 at 10:25 AM, Daniel Kraft via bitcoin-dev < > bitcoin-dev@lists•linuxfoundation.org> wrote: > >> Hi! >> >> I hope this is not a stupid question, but I thought I'd ask here first >> instead of opening a Github ticket (in case I'm wrong). >> >> With the recently merged "obfuscation" patch, content of the >> "chainstate" LevelDB is obfuscated by XOR'ing against a random "key". >> This is handled by CLevelDBWrapper's Read/Write methods, which probably >> cover most of the usecases. >> >> *However*, shouldn't it also be handled when iterating over the >> database? In particular, I would expect that the obfuscation key is >> applied before line 119 in txdb.cpp (i. e., while iterating over the >> coin database in CCoinsViewDB::GetStats). >> >> Is there a reason why this need not be done there, or is this an actual >> oversight? >> >> Yours, >> Daniel >> >> -- >> http://www.domob.eu/ >> OpenPGP: 1142 850E 6DFF 65BA 63D6 88A8 B249 2AC4 A733 0737 >> Namecoin: id/domob -> https://nameid.org/?name=domob >> -- >> Done: Arc-Bar-Cav-Hea-Kni-Ran-Rog-Sam-Tou-Val-Wiz >> To go: Mon-Pri >> >> >> _______________________________________________ >> bitcoin-dev mailing list >> bitcoin-dev@lists•linuxfoundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev >> >> > [-- Attachment #2: Type: text/html, Size: 3886 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bitcoin-dev] The new obfuscation patch & GetStats 2015-10-08 0:29 ` James O'Beirne @ 2015-10-08 5:14 ` Daniel Kraft 0 siblings, 0 replies; 4+ messages in thread From: Daniel Kraft @ 2015-10-08 5:14 UTC (permalink / raw) To: James O'Beirne; +Cc: bitcoin-dev [-- Attachment #1: Type: text/plain, Size: 678 bytes --] Hi James! On 2015-10-08 02:29, James O'Beirne wrote: > This has been confirmed as a bug. Thanks again for reporting. I've filed > a fix here (https://github.com/bitcoin/bitcoin/pull/6777), and will be > writing tests to prevent regressions. Thanks for the quick fix! I thought to submit a patch myself today in case the issue is confirmed as an oversight, but it is very nice to see that this is no longer necessary at all. :) Yours, Daniel -- http://www.domob.eu/ OpenPGP: 1142 850E 6DFF 65BA 63D6 88A8 B249 2AC4 A733 0737 Namecoin: id/domob -> https://nameid.org/?name=domob -- Done: Arc-Bar-Cav-Hea-Kni-Ran-Rog-Sam-Tou-Val-Wiz To go: Mon-Pri [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-08 5:15 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-07 17:25 [bitcoin-dev] The new obfuscation patch & GetStats Daniel Kraft 2015-10-07 23:32 ` James O'Beirne 2015-10-08 0:29 ` James O'Beirne 2015-10-08 5:14 ` Daniel Kraft
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox