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: 1387549#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: #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. db2b9fb#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:

monero/src/cryptonote_basic/cryptonote_basic.h

Line 175 in 9d7107c

VARINT_FIELD(unlock_time)

[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. cryptozoidberg/boolberry@4494852

[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? #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]

No one has explained how the ambiguity does not cause conflicts. If you’re going to claim that this is invalid, or that I am misunderstanding — shouldn’t there be a reason for such a conclusion?

The basis that the encoded representation in my example does not end in FF01 (Read left-to-right) doesn’t detract from the fact that any unlock time which does, would exhibit that behavior.

Should I open a new issue for this, and exclude the ar.stream_good() catalyst? I mean, ignoring the fact that the eof bit could be set, and the good() check wouldn’t catch it.

[who-biz]

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

Particularly tasteful way to say: “Go screw yourself and have fun cleaning up our mess.”

Seems that you have an inability for empathy except when you need it from others... Still, I hope this kind of conduct between developers comes to an end. The atmosphere your group creates is absolutely toxic.

[HorribleGelatinousBlob]

You said there was an issue, repeatedly. there is no issue. you have a history of fearmongering and fudding monero. Taking all this into consideration, go screw yourself seems appropriate

[HorribleGelatinousBlob]

And I should also point out you are not "cleaning up our mess". Monero, nor any of the contributors involved in monero owe you anything. It is your responsibility to make sure your shitcoin is kept up to date if you don't want to be left out in the cold when vulnerabilities are disclosed

[who-biz]

I have more history contributing to Monero than doing any sort of fear-mongering. Further, I’ve been cursed at, flamed, and slandered by you and others for doing what? Disclosing vulnerabilities properly in good faith?

Do you guys do anything in good faith? I don’t sink to the levels you’re operating on.

Edit: I don’t mean that to include people who have been reasonable. I’ll name selsta and SGP as two who have been rational, and gone further than simple efforts to mislead/dodge questions.

Answering my questions that pertain to this issue would be the preferred and productive road to take.

Food for thought: nobody defends the truth this way.

[HorribleGelatinousBlob]

what truth? you have been told you don't know what you're talking about. simple as that. the only truth here is that you have claimed an issue exists when it doesn't. You have not disclosed any vulnerabilities. people that do that don't get abused and cursed at. just you. that would tell most normal people that it is in fact you who is the problem. food for thought.

[who-biz]

And no one has explained a rationale behind the opinion that this is not an issue. Wonder why.

[lobstachub]

because you’re an ignorant child who is babbling nonsense.

your intentions is to bait into some nonsense. You are best left ignored

[HorribleGelatinousBlob]

How many times must you be told. your understanding of how a varint is serialized is wrong. There is nothing more to discuss. it is not an issue, because what you think is an issue is based on you misunderstanding how the code works. if you can demonstrate that there is a problem, reopen an issue and post an actual working proof of the issue. until then, there is nothing more to discuss

[who-biz]

Lol. No one told me that. I told you in my post.

Since we’re so hung up on variable length integers and my understanding, it’s safe to say that you guys all understand them very well.

Maybe someone could tell me how block height is encoded in this field, then, too? Also a varint? :)

[lobstachub]

Get a formal education

[who-biz]

@tchun Please stop the incessant typeface vomiting. Or use your actual account instead of a sock puppet for disparagement. You’re looking like a bytecoin sock puppet right now.

[lobstachub]

Child please. You’ve wasted enough people’s time.

[HorribleGelatinousBlob]

moneromoo said you don't understand

Thanks for the examole at last. It is incorrect, you are simply misunderstanding how it works.

jtgrassie also said you don't understand

You have just demonstrated your complete lack of understanding

I also am telling you you lack understanding. But it is not the responsibility of monero or it's contributors to educate shitfork plebs who demand answers. You now just sound like a child throwing a tantrum. I hope you do not expect to be taken seriously in the future.

[who-biz]

Thanks, Mr. Parrot. No, you guys don't care to explain things like this because you prefer simply saying "you don't understand". It's a cop-out. Look at how much time you're spending doing this instead of eradicating any "misinformation" I may be accidentally spreading. Instead, you prefer to avoid actually answering the questions, and proceed with ad hominem attacks which -- lets be honest -- are a sign of weakness. This leaves one to wonder why we would not want to answer the questions, despite criticizing others for their lack of understanding.

So again: I'll ask that you explain how timestamps within the unlock_time field are kept from conflicting with block_height values in interpretation. Specifically if an EOF is reached in the middle of that field's stream, or through conflict in the varint representation of a timestamp versus the size_t representation within block height.

[hyc]

@who-biz You clearly decoded the varints incorrectly. That's not an opinion, that's a fact. The obvious thing for you to do is go look up what the encoding rules of varints actually are. Not to keep haranguing everyone here to teach you how the code works.

[HorribleGelatinousBlob]

why do we need to eradicate your misinformation? just stop talking rubbish. you have been told go go away and do your research. there are many resources on the internet that will show you how this is done. don't want us spending time telling you that you are wrong. fine. stop posting rubbish.

as for explaining how unlock times are differentiated from block heights. read the code and you will find the answers you seek

[who-biz]

@hyc I'm agreeing with you that those encodings are incorrect. It says so in my original post.

But you know what, you guys are probably right! I've seen the error in my ways in asking direct questions. Clearly, no one here is reliable for disambiguating code or concepts. I guess from the outside, this maybe looks like you guys are the wrong people to ask questions. It's ok. I will source my information from elsewhere. I recommend others do the same.

[iamamyth]

Next time, you might want to read the github index page:

Built for developers
GitHub is a development platform inspired by the way you work. From open source to business, you can host and review code, manage projects, and build software alongside 36 million developers.

It's not meant for teaching people how code works, or how to code; it is for facilitating actual project development.

[HorribleGelatinousBlob]

This is what we have been saying. This is an issue tracker. This is not a classroom to answer questions you demand answers to. Finally you have decided to seek answers elsewhere, which is what we have all been saying for days. But the joy of open source code is that you can read the code and have your questions answered. Seems to me you just have not done that. So why would you expect anyone here to answer your questions, when from all appearances you appear to have put in none of your own effort. Seems all your effort is spent on arguing with people who don't really care about your opinion.

[who-biz]

I still strongly feel this is an issue. This wasn't an attempt to learn from you guys. It was an attempt to alert you to an issue. You instead, focused on a point that is irrelevant to the conceptual core of this issue. I agree, you all should be working to fix this, not questioning semantics in order to divert.

[HorribleGelatinousBlob]

thus is getting silly now. what you feel doesn't matter. show an issue. people dismiss you cause you say there is an issue, yet demonstrate a lack of understanding of how the code works and ask people to explain it to you. that makes your feelings invalid. no one is going to investigate any feelings from someone who doesn't know how the code works. you are just wasting yours and everyone else's time. I should also point out the programming in general is not subject to feelings. either something works or it doesn't. to that end, there appears to be no issue with how the code works, otherwise you would be able to show how it doesn't work. constantly replying here saying you feel like something is wrong is just pointless for the reasons I just mentioned

[who-biz]

@HorribleGelatinousBlob why are you even polluting this discussion? You’re not a contributor to Monero.

[lobstachub]

Why are you still replying to this when it’s closed

[HorribleGelatinousBlob]

babysitting the children is my contribution to monero.

[who-biz]

Because the issue still persists. Its okay. I’ll open a new one, if that’s what we prefer.

[HorribleGelatinousBlob]

the issue was closed as invalid because you demonstrated a lack of understanding of how the code works and could not identify an issue. You are free to open another issue, however without clearly articulating the issue, it will end in the same way

[who-biz]

I've a feeling it will end the same way, regardless :)