Unlock time's dual interpretation causes conflict in Feb 2019 with recent code changes

[who-biz]

See below

[who-biz]

I commented this on the commit, but since no one seems quick to respond to that route of action -- here it is again, for ease of reference:

This change is problematic: https://github.com/monero-project/monero/commit/1387549e905fc206426d3099b069bd28df0aad71#diff-1f77146989e4bf145cab519a8adbd48aR215

Since XMR uses .xz-style encoding (and 0x01 signals the termination of concatenated data), changing this looks like its going to make the timestamp for say, Feb. 20th or so, conflict with block 1,550,6xx (or wherever has a timestamp that reflects a (block height)*(10³))...

[moneromooo-monero]

By "dual interpretation", do you mean the fact that's is a block number or timestamp based on value ?

Where do you think monero uses "xz-style encoding" ? It might, I've no idea what it does. But I'm not going on your wild goose chase this time. Varints (unlock time is a varint) are encoded with the high bit cleared at the end.

For the conflict, explain what you think the conflict is.

[moneromooo-monero]

And I don't read email, so I don't get notifications. Comment in PRs if you found something or I won't see it unless by chance.

[who-biz]

By "dual interpretation", do you mean the fact that's is a block number or timestamp based on value ?

I mean the fact that the unlock_time in the block structure is a varint field. This field is interpreted as block height in some scenarios, and in others (in simplewallet for example if the field exceeds a value of 500000000) as a timestamp. Usually, this is "whichever comes first" that dictates it. That's incredibly problematic in many areas.

Where do you think monero uses "xz-style encoding" ?

Within its variable length integers. These aren't as the description in varint.h implies. See here: https://github.com/monero-project/monero/issues/2340

Edit: I misspoke in saying the xz-style encoding is what makes the difference here. It’s the behavior above.

The conflict is that with the ar.stream_good() at the end of the KV_SERIALIZE_END macro, the varint interpretation changes. Now, the timestamp of 1556000(000) for example, will be intrepreted the same as a block height of 1,556,000.

If we're at that block height, then the timestamp is 1555839480 aka 2018-04-21 09:38:00 UTC.

1556000000 or 1555839480 . . . which comes first?

You can call it a wild goose chase, but I'd call it more like me making two responsible disclosures and you guys spitting in my face. Not to mention, breaking HackerOne's terms of service (in the first one at least). It's cool, like I said -- I didn't come there to get paid. But don't get all high and mighty about it.

I might add, as I’ve said to you before... that the flaw behind the first disclosure then became your “ledger bug”.

[jtgrassie]

I maybe missing something here but ar.stream().good(); is not writing the data, it's signaling whether the serialization happened ok or not. So it's not clear to me what you're trying to get at.

[who-biz]

@jtgrassie Yep. I know that, and so do both of you. https://github.com/monero-project/monero/commit/db2b9fba651670a9c8f86c316f36bc9d9dbb82cc#diff-5be7f4b15905c17dfe82ebe394ffa10e

Edit: Well, serializing the data is kinda re-writing it. So apart from that -- sure.

I didn't think changing the way things were interpreted, sounded like I was implying anything about writing the data.

[jtgrassie]

Your are saying:

The conflict is that with the ar.stream_good() at the end of the KV_SERIALIZE_END macro, the varint interpretation changes.

And what I'm getting at is that ar.stream().good(); does not change any interpretation. unlock_time is a varint regardless if the data it is holding can be used as a block time or timestamp. It's still just a 64 bit int encoded as a varint. The serialization doesn't care.

[stoffu]

@who-biz

The conflict is that with the ar.stream_good() at the end of the KV_SERIALIZE_END macro, the varint interpretation changes. Now, the timestamp of 1556000(000) for example, will be intrepreted the same as a block height of 1,556,000.

1556000 and 1556000000 are distinctly varint encoded as a0fc5e and 80dafae505, respectively. Unlock time less than CRYPTONOTE_MAX_BLOCK_NUMBER (=500,000,000) is regarded as the block height as specified in the protocol (Blockchain::is_tx_spendtime_unlocked in blockchain.cpp).

What you've been saying here doesn't make any sense to me.

[who-biz]

It cares here: https://github.com/monero-project/monero/blob/master/src/serialization/binary_archive.h#L169-L175

And a few lines further down, here:

  template <class T>
  void serialize_uint(T v)
  {
    for (size_t i = 0; i < sizeof(T); i++) {
      stream_.put((char)(v & 0xff));
      if (1 < sizeof(T)) v >>= 8;
    }
  }

[who-biz]

1556000 and 1556000000 are distinctly varint encoded as a0fc5e and 80dafae505, respectively

@stoffu I agree. The issue isn't their representation in hexidecimal. It's the fact that the mask and shift doesn't happen, if the stream doesn't signal the end of data with a terminating bit.

[jtgrassie]

@who-biz

It cares here...

template void serialize_uint(T v) {...

No it doesn't. That is uint serialization not varint. unlock_time is defined as a varint field so that is how it's encoded/decoded. ref: https://github.com/monero-project/monero/blob/9d7107c870d88885f43a8fd65739b1b70a5af668/src/cryptonote_basic/cryptonote_basic.h#L175

[stoffu]

@who-biz

I see your argument as non-issue unless you come up with an example tx blob that actually breaks the system.

[who-biz]

@stoffu With all due respect, thats a bit of a cop-out.

Although, at this stage, it seems already done. Just thought I'd let ya know that I find it concerning.

[stoffu]

@who-biz

It's not a cop-out, it's just you failing to identify what issue there might exist.

[moneromooo-monero]

There is a slight change in interpretation, but you get to explain why it is exploitable. AFAIK, before the change, a value of 0x00 would be read as 0, and a value of 0x80 would be read as 0. With the patch, the first one errors out. Off the top of my head, I did not actually test. Something being off by a factor of 1000 somewhere seems implausible to me. If your claim is different, then you do get to explain how exactly.

[who-biz]

@stoffu Or perhaps the explanation as much for those who don't see binary every day.

It does look like someone over at Boolberry may have figured this one out already. Commits happened the same week as the ones in XMR. Weird.

[moneromooo-monero]

STOP being vague. Point to EXACT patches. Don't make us guess what you're saying, looking for patches somewhere with so little information. I will not waste time again unless information is given, not hinted at.

[who-biz]

I'm not referring to patches. I'm referring to the number conflicts. https://github.com/cryptozoidberg/boolberry/commit/449485220b05bc1927378a09cc7fdd8e10199ddb

[moneromooo-monero]

OK, you said commits, not patches. Fine. I don't see what that commit has to do with varints. Again, please be specific.

[who-biz]

things look strange to me with the unlock time changes. I just think that liberal interpretation of block structure in a field like that is not a good idea mixed with untested changes.

Take for example the fact that if the fail bit gets set earlythat you could be truncating a timestamp into block height.

[moneromooo-monero]

Ah, so there is one bit of information here. You think that the fail bit can be set early. If this happens before it should, then yes, you can ready the wrong value. Can you point out where you think that can happen ?

[who-biz]

I am not saying it’s exploitable. I am saying I think this looks like it will cause problems.

[HorribleGelatinousBlob]

The question was

Can you point out where you think that can happen ?

This issue should be closed as invalid if this simple piece of information can't be provided. To just say

...this looks like it will cause problems.

Is not meaningful, helpful or useful in any way. This is an issue tracker for actual issues. Not code you "think" might cause a problem.

[HorribleGelatinousBlob]

I should also add a link to a recent reddit post from the OP, where he claims that hashing a block with the same nonce twice would expose your private keys. A direct quote:

security breaks completely and can reveal your private keys if you hash the same block twice with the same nonce.

Draw what conclusions you may regarding the reliability and knowledge of the person posting this issue

[who-biz]

I invite that. Why don’t you reveal the rest of that post? It’s clear that I’m not talking about hashing a block there. Chacha20 is a stream cipher and yes it’s breaks when you’re encrypting a stream larger than 2⁷⁰ bytes. Nothing in code guards against that except a comment that says it’s “user’s responsibility” to stop there.

But you’re detracting from the issue with skewed quotes that lack context . Can we stay on topic please instead of attempting to smear someone like me

[who-biz]

Feel free to close the issue but I think it warrants discussion.

[HorribleGelatinousBlob]

OK. back to topic. The link to the thread is there if anyone cares to read it. @moneromooo-monero asked for a specific example of how what you say can happen and the effect. he is talking about an actual code example, or an example transaction that will break the current code. speculation has no place here. either there is a valid issue which you can demonstrate or there is not. there is no need to further this discussion without that.

[who-biz]

Since when does discussion have no place here? https://github.com/monero-project/monero/issues/4533 There are a lot of prior examples of the issue tracker being used for discussion of past present and future issues.

[HorribleGelatinousBlob]

The title of this issue does not state this is a discussion. it states there is an issue relating to the interpretation of timestamps in block lock times. now you are being asked to demonstrate that. why do you refuse to provide a demonstration? what is the point of discussing code that appears to work fine with no proof provided to the contrary? why do we need a discussion if everything is working as it should? And if it isn't, why won't you show us how it is not working properly. You are just wasting everyone's time.

[who-biz]

This is an issue, in my opinion. You don't agree. At which point, the issue became a discussion... It's first grade, Spongebob.

[who-biz]

For starters, our window to examine would begin at the value of 1341379000 since the code has the genesis block set to that value, retroactively, for some reason.

[HorribleGelatinousBlob]

the problem is "your opinion". Either it is an issue, or it isn't. Your feelings on the matter are irrelevant. So the request remains open. Provide an example of how the code is broken, or at least point to some code that is relevant to the discussion. you still have done neither and try to continue this conversation with nonsense. Just provide something solid we can discuss, otherwise the only reasonable conclusion is that you are trolling.

[stoffu]

If the fail bit is read early and the block blob wasn't fully and correctly received, then the block will surely be rejected when validating the PoW.

@who-biz

I just think that liberal interpretation of block structure in a field like that is not a good idea mixed with untested changes.

What do you mean by "liberal interpretation"?

At this point your objective seems to be to draw attention and waste people's time.

[who-biz]

@stoffu True for current blocks. Not true for blocks that fall in the compiled hashes area that were also keeped_by_block. These are in the past and assumed valid. Not to mention, if the timestamp seems like a valid block, or vice versa, that nothing in a sanity check would catch them as they appear valid.

[HorribleGelatinousBlob]

You have now been asked many times to simply provide a demonstration of how this dual purpose field may be interpreted incorrectly. @stoffu is correct. you are wasting everyones time. If you are so sure there is an issue, it should be trivial to demonstrate it. Yet you refuse repeated requests to provide a demonstration or even a reference to some code you deem problematic. No one is able to follow the vague description you are providing and you are not providing the requested information to discuss this further. This is simply a troll, where it appears you have a history of trolling Monero.

[iamamyth]

@stoffu True for current blocks. Not true for blocks that fall in the compiled hashes area that were also keeped_by_block. These are in the past and assumed valid.

No. Nice try, but no. If they're in the past, it should be easy for you to provide a demonstration.

Not to mention, if the timestamp seems like a valid block, or vice versa, that nothing in a sanity check would catch them as they appear valid.

You mentioned it, and it's utter nonsense.

Let me simplify the conversation: You are talking to a toddler who has learned to type. You have assumed this individual is learned, on account of being able to type. At some juncture, you must realize, if you continue on this path, your entire project is doomed, because many individuals can produce words on a page, and, if you respond to all of them, you lose time in building software. Do as you will.

[moneromooo-monero]

It's going the way as previous reports. If it's about blocks within the compiled hash area, then you have to say it in the first place, or you are indeed wasting our time again. If there's a problem, explain it. Don't say there's a problem somewhere but withhold information such as the above and let us try to guess at the problem. Reporting a problem is not a game of blindfolded darts. So if you are trying to report a problem, as I assume you are since you are posting here, give all the details so we can assess the validity of your claim.

[HorribleGelatinousBlob]

This issue seems to be based on opinions and speculation and not in reality. when you say that it's your opinion the code is liberally interpreting data, then you just have no idea what you're saying. Remember that this is the same person who declared on reddit that

security breaks completely and can reveal your private keys if you hash the same block twice with the same nonce.

I think this tells us everything we need to know.

[who-biz]

If it's about blocks within the compiled hash area, then you have to say it in the first place, or you are indeed wasting our time again.

That is hardly what I’m doing. Stoffu mentioned a block failing validation for the PoW. I was merely making the point that we are talking about historical, checkpointed blocks. The PoW validation argument does not hold up when we are speaking about historicals. You can call it blindfolded darts, but I’m merely speaking to your counterpoints. I’ve been busy with 9 vulnerabilities you guys dropped at once... so my apologies. Give me a few minutes and I’ll post a full description of what I am getting at.

[lobstachub]

Don’t give this child any attention

[who-biz]

For a "non-issue" this is being defended pretty vehemently. I really wasn't trying to be adversarial, and have been attacked by many of you. All of which, upon the sole basis of your opinion: that the issue I speak of is non-existent.

This sort of behavior results in problems like the ledger bug impacting users before a choice is made to do anything about it. Rather than a mature response, the report I filed on the same underlying issue (lack of accounting for R') was met with profanity, instead of a "gracious response" as your VRP states. As a result, I'm sure you nearly gave the three users that had the ledger issue a heart attack. But yes, lets write off more reporters to be "fucking trolling," as you so eloquently put it on HackerOne (against their ToS, btw).

Are there words I've said in the past that were incorrect? Yes. Of course. When we find out we're wrong, we learn and grow. When a developer won't admit that, and doesn't amend their perspective, the network and users suffer instead. Humans have been wrong more than anything else in history. Myself included. So chill out.

Back to the topic at hand... (Y'all make one exhausting gaggle of naysayers)

Let's suppose we have a timestamp = 0x59FF01FF. That's during the 5th of November, 2017 UTC.

Correct me if I'm wrong... haha, of course you will :-) ... But this timestamp, as well as any varint which terminates in FF needs an additional '0x01' afterward, to account for the 7-shift and our final bit.

Hex:         0xFF          |      0x7FF     |      0x59FF01FF
Encoded:       0xFF01      |     0x7FF01  |      0x59FF01FF01

Remember, Remember our block structure, we have:

     | tx version | unlock time |   vin    | tx type | input height | ...... and so on. 

Also, note from CNS004 -

  - version: Version defines the transaction parsing rules (i.e.
  transaction content) and is incremented by each transaction format
  update. **Parsing transactions with transaction_prefix of an unknown
  version is not safe because transaction content could be
  misinterpreted.** Currently only transactions of version 1 are
  defined.

Recall that this is the same reason we define a count after a specified tag within the tx_extra if the length of that field is not implicit within the context of the field itself. When we have a dual context, this creates issues and we have plenty of design examples to look at, in order to know when and where this matters most. Most of these reside in the prefix, along with unlock time.

Let's hypothetically assume that someone decided to mine a block at height 511 (0x1ff). They've also decided that they wanted to wait until November 5th, 2017 to actually spend those outputs.

              | tx version |     unlock time   |   vin    | tx type | input height |  
Or:                   0x01  |     0x59ff01ff     |  0x01  |   0xff      |    0x1ff
Encoded as:        0x01     |   0x59ff01ff01     |  0x01  |     0xff01    |    0x1ff01

Doesn't it look like things will get awfully confusing if we have a failbit truncate that unlock _time varint into the value 0x59ff (now a block height of 23039) instead?

Edit: Before the crowd gets going, I know that protobuf changes the representation of these hex values more than I'm illustrating. This is for explanatory purposes only.

[jtgrassie]

But this timestamp, as well as any varint which terminates in FF needs an additional '0x01' afterward, to account for the 7-shift and our final bit.

Incorrect.

1. You have just demonstrated you do not know how to encode/decode a varint. (0x59FF01FF gets encoded to 0xFF83FCCF05 not 0x59FF01FF01)
2. You have just demonstrated your complete lack of understanding regarding the stream failbit in that a tx that fails serialization (failbit gets set), the whole structure is invalid - it failed serialization.

Therefore you have still failed at explaining any potential issue.

Before the crowd gets going, I know that protobuf changes the representation of these hex values more than I'm illustrating. This is for explanatory purposes only.

When you are trying to say there is a problem with encoding/decoding but then base your whole thesis on an incorrect illustration and assumption of how it works, proves beyond doubt, you have no merit debating this.

[lobstachub]

Don’t give this child any attention

[jtgrassie]

Yes, you're right. I'm done.

[who-biz]

See the bottom of my post. I wrote it the way I did for comprehension for readers, outside of your developers. Say, the issues I raised about ZeroMQ and MiniUPnP.... @jtgrassie didn't you PR some of those suggestions? After going on and on about me on reddit? Come on. Address the relevance in concept. A very simple fix would be to include a flag to denote if a transfer has an unlock_time or an unlock_height. Alternatively, a counter for the number of significant figures to follow.

[who-biz]

If someone cares to elaborate on precisely how the ambiguity somehow does not matter, that would be a very sufficient response. Supporting points for why a counter or a flag wouldn't be less error-prone would be great, too.

[HorribleGelatinousBlob]

no one is vehemently defending anything except to have an issue tracker free from idiocy. fake issues detract from real ones and it seems that once again you are claiming the sky is falling with no evidence except some childish scribbles. you clearly don't understand how the code works. you are not qualified to an opinion on whether or not there is an issue

[lobstachub]

Don’t give this child any attention

[who-biz]

Let me know when the grown-ups are back to chat.

[HorribleGelatinousBlob]

The grown ups have come, determined your "issue" to be a non-issue, determined that you do not know what you're talking about and now left again.

You should probably go and work on your 9 vulnerabilities and come back when you have something different to go chasing shadows about.