View Issue Details

IDProjectCategoryView StatusLast Update
0022898mantisbtsecuritypublic2019-08-25 12:36
Reporterwally68 Assigned Todregad  
PrioritynormalSeveritymajorReproducibilitysometimes
Status closedResolutionfixed 
Product Version2.2.1 
Target Version2.22.0Fixed in Version2.22.0 
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

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

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

Related Changesets

MantisBT: master ad42c3ca

2019-08-10 13:21

dregad


Details Diff
Prevent email about private note to unprivileged users

In email_collect_recipient(), the logic to exclude users who can't see
bugnotes relied on comparing the issue's last updated timestamp with the
bugnote's date.

Since these dates are not necessarily equal as they are updated
separately when a bugnote is added, this may result in a race condition
causing a notification e-mail about a new private bugnote to be sent to
users not authorized to see them.

Since email_collect_recipient()'s $p_bugnote_id parameter is always null
except for 'bugnote' notifications, the date check is not necessary; it
is sufficient to check that $p_bugnote_id is not null.

Fixes 0022898
Affected Issues
0022898
mod - core/email_api.php Diff File