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

Invalid secret request keeps following valid one from being processed #3001

Closed
jomuel opened this Issue Nov 12, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@jomuel
Collaborator

jomuel commented Nov 12, 2018

Another fuzz test finding, see test_failing_path_4 in the open fuzz testing pr (#2887):

ReceiveSecretRequest is not processed if an invalid secret request for the same transfer happened before.

@palango

This comment has been minimized.

Collaborator

palango commented Nov 13, 2018

A short walkthrough of the issue that @jomuel found:

  • When the initiator receives a ReceiveSecretRequest it calls the initiator to handle it:

    elif type(state_change) == ReceiveSecretRequest:
    channel_identifier = payment_state.initiator.channel_identifier
    channel_state = channelidentifiers_to_channels[channel_identifier]
    sub_iteration = initiator.handle_secretrequest(
    payment_state.initiator,
    state_change,
    channel_state,
    pseudo_random_generator,
    )
    iteration = iteration_from_sub(payment_state, sub_iteration)

  • The initiator checks if the request if valid, and if not returns (None, [EventPaymentSentFailed(...)]):

    elif not is_valid_secretrequest and is_message_from_target:
    cancel = EventPaymentSentFailed(
    payment_network_identifier=channel_state.payment_network_identifier,
    token_network_identifier=channel_state.token_network_identifier,
    identifier=initiator_state.transfer_description.payment_identifier,
    target=initiator_state.transfer_description.target,
    reason='bad secret request message from target',
    )
    iteration = TransitionResult(None, [cancel])

  • This in turn is handles in the node and the task gets removed from the payment mapping:

    if sub_iteration and sub_iteration.new_state is None:
    del chain_state.payment_mapping.secrethashes_to_task[secrethash]

My question is how this should be handled:

  1. After a invalid request all subsequent ReceiveSecretRequest should be ignored
  2. Allow a certain number of ReceiveSecretRequests before invalidating the task (or let it run until the payment times out
@hackaugusto

This comment has been minimized.

Collaborator

hackaugusto commented Nov 13, 2018

The rationale here is, since the message is authenticated, we know it's the target who sent the message, if he sent an invalid SecretRequest we do not assume it's a bug, but we assume it is an attack, therefore we drop the payment completely.

I think this is a bug, because in the past that would be fine, before there was no RemoveExpiredLock message. With the introduction of that message, we need the initiator task to be living to remove the lock, otherwise we are going to lose capacity. Note, however, that this should not manifest itself as a nonce synchronization problem (from what I see in your description)

Edit: So, in short, my opinion is that we should reject subsequent invalid secret requests. The reason is simple, we will never be able to differentiate from bugs to attacks, and then I believe assuming the later is safer.

@palango

This comment has been minimized.

Collaborator

palango commented Nov 13, 2018

So, in short, my opinion is that we should reject subsequent invalid secret requests.

Ok, so then there is nothing to do here. If the ReceiveSecretRequest is invalid it gets removed from the payment mapping and all subsequent ones will be ignored.

@palango palango closed this Nov 13, 2018

@palango

This comment has been minimized.

Collaborator

palango commented Nov 13, 2018

I misinterpret Augustos second comment, the payment task should not be deleted as there might still be locks.

@palango palango reopened this Nov 13, 2018

@jomuel

This comment has been minimized.

Collaborator

jomuel commented Nov 13, 2018

Meanwhile I removed test_failing_path_4, and I want to keep it that way since it does not test for the bug @hackaugusto described. The behaviour observed in test_failing_path_4 was correct and is now tested for in the initiator fuzz test.

palango added a commit to palango/raiden that referenced this issue Nov 13, 2018

palango added a commit to palango/raiden that referenced this issue Nov 13, 2018

Extend tests for raiden-network#3001
[no ci integration]

@palango palango removed the investigating label Nov 13, 2018

palango added a commit to palango/raiden that referenced this issue Nov 14, 2018

Extend tests for raiden-network#3001
[no ci integration]

hackaugusto added a commit that referenced this issue Nov 14, 2018

hackaugusto added a commit that referenced this issue Nov 14, 2018

Extend tests for #3001
[no ci integration]

rakanalh added a commit to rakanalh/raiden that referenced this issue Nov 16, 2018

rakanalh added a commit to rakanalh/raiden that referenced this issue Nov 16, 2018

Extend tests for raiden-network#3001
[no ci integration]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment