View Issue Details

IDProjectCategoryView StatusLast Update
0022898mantisbtsecuritypublic2019-08-09 20:41
Reporterwally68 Assigned Todregad  
PrioritynormalSeveritymajorReproducibilitysometimes
Status assignedResolutionopen 
Product Version2.2.1 
Target Version2.22.0Fixed in Version 
Summary0022898: Email for a new private bugnote was send to a non authorized reporter
Description

We have found a strange problem with private bug-notes:
A reporter has received a private bug note from a developer while the reporter is not authorized to see any private notes or bugs.

After some code digging, we found this in core/email_api.php::email_collect_recipients, around line 461:

        # exclude users who don't have at least viewer access to the bug,
        # or who can't see bugnotes if the last update included a bugnote
        if( !access_has_bug_level( config_get( 'view_bug_threshold', null, $t_id, $t_bug->project_id ), $p_bug_id, $t_id )
         || ( $t_bugnote_id !== 0 &&
                $t_bug_date == $t_bugnote_date && !access_has_bugnote_level( config_get( 'view_bug_threshold', null, $t_id, $t_bug->project_id ), $t_bugnote_id, $t_id ) )
        ) {
            log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, drop @U%d (access level)', $p_bug_id, $t_id );
            continue;
        }

The recipient for a bugnote email is excluded if the bug date is equal to the bugnote date and the access level is wrong. You use the lastmod-timestamp from the bug and the bugnote to differ between a bug email and a bugnote email.

The timestamp for a bugnote is created by db_now() in core/bugnote_api.php::bugnote_add. The timestamp for the bug is created by db_now() in core/bug_api.php::bug_update_date. The function bug_update_date is called from bugnote_add.

In our opinion there is a potential gap to create two different timestamps for the bugnote and the bug especially on slow machines.

As a possible solution, function core/bug_api.php::bug_update_date may be extended with a default parameter $p_last_modified = 0 and the call from bugnote_add would set the timestamp from the bugnote as a parameter to bug_update_date.

kind regards
Andreas

TagsNo tags attached.

Relationships

has duplicate 0023492 closedatrol Due to condition race email may be sent to reporter where it should not 

Activities

wally68

wally68

2017-05-19 04:57

reporter   ~0056905

Instead of testing the bug timestamp against the bugnote timestamp in <b>core/email_api.php::email_collect_recipients</b> the function parameter <b>$p_notify_type</b> could be tested against 'bugnote' right?

wuttke

wuttke

2018-02-14 15:51

reporter   ~0058870

Last edited: 2018-02-19 10:39

View 2 revisions

function bug_update_date( $p_bug_id, $p_last_modified = 0 ) {
    db_param_push();
    $t_query = 'UPDATE {bug} SET last_updated=' . db_param() . ' WHERE id=' . db_param();
    if ($p_last_modified == 0) {
        $p_last_modified = db_now()
    }
    db_query( $t_query, array( $p_last_modified, $p_bug_id ) );

    bug_clear_cache( $p_bug_id );

    return true;
}

line 306:

    # update bug last updated
    if( !$p_skip_bug_update ) {
        bug_update_date( $p_bug_id, $c_last_modified );
    }

EDIT (dregad) fix markdown

wuttke

wuttke

2018-02-14 15:53

reporter   ~0058871

https://github.com/wuttke/mantisbt/commit/acfc7d444a016b440bd861da9cdb31c0be2a3e64

we'll check and then I'll create a pull request

wuttke

wuttke

2018-02-15 02:54

reporter   ~0058873

https://github.com/mantisbt/mantisbt/pull/1299

vboctoradmin

vboctoradmin

2018-02-28 23:19

administrator   ~0059059

Do we really need to timestamp check? Should we base this on the fact that the change is about a bugnote addition and having a bugnote id?

wally68

wally68

2018-03-01 02:23

reporter   ~0059060

@vboctoradmin
That's what I mean in my note 0022898:0056905. It would be much simpler to implement, right?

vboctor

vboctor

2018-03-29 19:08

manager   ~0059356

@wuttke what do you think?

Herr.mullepulle

Herr.mullepulle

2019-07-29 06:52

reporter   ~0062446

@vboctor
What is the status of this bug?

We just stumbled across it in version 2.18.1.:
Person A (who is not authorized to view private notes) created a ticket. Person B (Developer) left a private note in this ticket (so the intention is that Person A cannot read the note).
Still, Person A receives an email informing about the new note in his ticket. He cannot see the note in Mantis itself but can view the content in the email.

Am I correct that this problem is the same as in this bug here?

dregad

dregad

2019-07-29 11:52

developer   ~0062447

Well we never got a response to my review comment in the PR, nor to vboctor's question below, so you should really be asking @wuttke...

Herr.mullepulle

Herr.mullepulle

2019-08-09 05:06

reporter   ~0062542

@dregad

Unfortunately it seems that @wuttke is no longer active around here. It would be a shame if this issue just kept sitting here without any change because of that.

Is there any way for you mantis developers to implement a fix for this? At least for my organisation it would be a tremendous help, since we want to rely on our private bug notes to remain private ;)

dregad

dregad

2019-08-09 10:45

developer   ~0062545

@Herr.mullepulle you're right.

The question is, whether the proposed fix based on adjusting the timestamp to match is the proper solution (in which case it's a simple matter of adjusting @wuttke's pull request based on my review, which could be done easily), or if we should implement the alternative, seemingly more robust approach suggested by @wally68 and @vboctor to rely on notification type.

dregad

dregad

2019-08-09 11:09

developer   ~0062547

Confirming the issue.

To reproduce: the race condition can be forced by adding a sleep( 2 ); statement in bugnote_add(), right after initialization of $c_last_modified.

dregad

dregad

2019-08-09 20:21

developer   ~0062551

Last edited: 2019-08-09 20:41

View 3 revisions

Alternate PR https://github.com/mantisbt/mantisbt/pull/1541 to the one proposed in 0022898:0058873

I think it might still be a good thing to ensure the bugnote's timestamp is effectively always equal to that of the parent bug's last_updated timestamp

Issue History

Date Modified Username Field Change
2017-05-18 06:51 wally68 New Issue
2017-05-19 04:57 wally68 Note Added: 0056905
2017-10-17 10:04 atrol Relationship added has duplicate 0023492
2018-02-14 15:51 wuttke Note Added: 0058870
2018-02-14 15:53 wuttke Note Added: 0058871
2018-02-15 02:54 wuttke Note Added: 0058873
2018-02-19 10:39 dregad Note Edited: 0058870 View Revisions
2018-02-28 23:19 vboctoradmin Note Added: 0059059
2018-03-01 02:23 wally68 Note Added: 0059060
2018-03-29 19:08 vboctor Note Added: 0059356
2019-07-29 06:52 Herr.mullepulle Note Added: 0062446
2019-07-29 11:52 dregad Note Added: 0062447
2019-08-09 05:06 Herr.mullepulle Note Added: 0062542
2019-08-09 10:01 dregad Description Updated View Revisions
2019-08-09 10:03 dregad Description Updated View Revisions
2019-08-09 10:45 dregad Note Added: 0062545
2019-08-09 11:09 dregad Status new => confirmed
2019-08-09 11:09 dregad Note Added: 0062547
2019-08-09 20:19 dregad Assigned To => dregad
2019-08-09 20:19 dregad Severity minor => major
2019-08-09 20:19 dregad Status confirmed => assigned
2019-08-09 20:19 dregad Category email => security
2019-08-09 20:19 dregad Target Version => 2.22.0
2019-08-09 20:21 dregad Note Added: 0062551
2019-08-09 20:40 dregad Note Edited: 0062551 View Revisions
2019-08-09 20:41 dregad Note Edited: 0062551 View Revisions