Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#5646 closed defect (fixed)

Many functions log before checking for proto violation

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: #5643 Points:
Reviewer: Sponsor:

Description

rend_mid_establish_intro():

  log_info(LD_REND,
           "Received an ESTABLISH_INTRO request on circuit %d",
           circ->p_circ_id);

rend_mid_introduce():

  log_info(LD_REND, "Received an INTRODUCE1 request on circuit %d",
           circ->p_circ_id);

rend_mid_establish_rendezvous():

  log_info(LD_REND, "Received an ESTABLISH_RENDEZVOUS request on circuit %d",
           circ->p_circ_id);
< [censored]> check proto was places exactly to filter that. some one encodes something before it, that is bug.
< asn> check proto?
< asn> ye, rend_mid_rendezvous() does the stupid encoding thing before checking for proto violation, like you said.
< [censored]> most of that funcs is wrong.
< [censored]> some logs about recved stuff. why if it wrong cell in wrong circ?
< asn> then you get two logs
< asn> one that says "received..." and another that says "rejecting..." 
< asn> in the case of rend_mid_establish_intro() for example
< [censored]> why? another funcs can't report about recved cell without encode. no one funcs should log anything before checks.
< asn> ok. i'm more than fine with "checks first, everything afterwards (including logs)"

Child Tickets

Change History (6)

comment:1 Changed 8 years ago by arma

When we are changing this, it would be great to log whatever we used to log when declaring the protocol violation. Part of the reason those initial logs are there is to let people reading the logs follow what is happening (even if it shouldn't be happening!)

comment:2 in reply to:  1 Changed 8 years ago by asn

Replying to arma:

When we are changing this, it would be great to log whatever we used to log when declaring the protocol violation. Part of the reason those initial logs are there is to let people reading the logs follow what is happening (even if it shouldn't be happening!)

OK. This means that in cases like rend_mid_introduce(), we also have to print p_circ_id in the protocol violation log, which is not too different from the current situation. If we consider printing p_circ_id totally innocent, then maybe we should leave such code as is, instead of tweaking it to add the p_circ_id in the protocol violation log.

If the above paragraph sounds like a good idea, then the only offender of this ticket that I can find is the:

reason = (uint8_t)cell->payload[0]

in command_process_destroy_cell():

  circ = circuit_get_by_circid_orconn(cell->circ_id, conn);
  reason = (uint8_t)cell->payload[0];
  if (!circ) {
    log_info(LD_OR,"unknown circuit %d on connection from %s:%d. Dropping.",
             cell->circ_id, conn->_base.address, conn->_base.port);
    return;
  }

comment:3 Changed 8 years ago by asn

Status: newneeds_review

Please see branch bug5646 in https://git.gitorious.org/mytor/mytor.git.

Please also make a quick check in cell processing functions to see if I missed anything.

If you merge and mark this ticket as closed, also close the parent ticket (#5643).

comment:4 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged. Closing, based on arma's rationale. Please comment with explanation if you disagree with arma's rationale.

comment:5 Changed 8 years ago by nickm

Keywords: tor-client added

comment:6 Changed 8 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.