Closed Bug 1214310 Opened 9 years ago Closed 9 years ago

Markdown code blocks can display comments from other bugs

Categories

(Bugzilla :: User Interface, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 6.0

People

(Reporter: altlist, Assigned: altlist)

References

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

I am seeing cases where Bugzilla is displaying ticket A with code blocks from ticket B.  

I believe the issue is that Markdown::_removeFencedCodeBlocks uses the global variable, @code_blocks.  Said variable should instead be defined as an instance variable.
If that is true, then this is a security bug. If Bugzilla starts displaying comments from other bugs, we are in trouble. Fortunately, no Bugzilla release contains the Markdown code yet.

I guess you need mod_perl to reproduce the issue?
Group: bugzilla-security
Version: unspecified → 5.1
Summary: Markdown code blocks can display comments from other tickets → Markdown code blocks can display comments from other bugs
Target Milestone: --- → Bugzilla 6.0
How could that happen? @code_blocks is declared with 'my'.
I'm using mod_perl.  I'm currently using a patch that treats code_blocks as an instance variable, similar to Bug::status.  Is that the correct approach?
Attached patch bug-1205072-code-blocks (obsolete) — Splinter Review
For what it's worth, I've attached a patch that appears to solve my needs.  This does include updates to ticket 1205072, as code block handling was not working cleanly to begin with.
Attachment #8674330 - Flags: review?(koosha.khajeh)
Comment on attachment 8674330 [details] [diff] [review]
bug-1205072-code-blocks

>+++ Bugzilla/Markdown.pm	2015-09-17 15:17:19.689174000 -0500

> our %g_escape_table;
>-my @code_blocks;

You cannot use |my @foo| outside a subroutine in a .pm file. This doesn't work with persistent environment. You must write |our @foo| instead. This is why you are in trouble with mod_perl.
(In reply to Albert Ting from comment #4)
> This does include updates to ticket 1205072

Please do not mix patches from different bugs. It's a mess to review and to keep track.
Depends on: 1154116
Keywords: regression
Is it sufficient to just use "our" instead of "my"?  Or should the array be treated as an instance variable?
(In reply to Albert Ting from comment #7)
> Is it sufficient to just use "our" instead of "my"?  Or should the array be
> treated as an instance variable?

Using "our" should do it. Also, please remove @Bugzilla::Markdown::EXPORT = qw(new); No module should export new().
Attached patch v2 (obsolete) — Splinter Review
Updated patch that uses "our"
Attachment #8674330 - Attachment is obsolete: true
Attachment #8674330 - Flags: review?(koosha.khajeh)
Attachment #8674708 - Flags: review?(LpSolit)
To be sure, does this fix your problem?
(In reply to Frédéric Buclin from comment #5)
> Comment on attachment 8674330 [details] [diff] [review]
> bug-1205072-code-blocks
> 
> >+++ Bugzilla/Markdown.pm	2015-09-17 15:17:19.689174000 -0500
> 
> > our %g_escape_table;
> >-my @code_blocks;
> 
> You cannot use |my @foo| outside a subroutine in a .pm file. This doesn't
> work with persistent environment. You must write |our @foo| instead. This is
> why you are in trouble with mod_perl.

This is completely not true.
What is true, is that you cannot use a file-scoped my inside a .cgi script. Inside a .pm is fine. If it wasn't, very many cpan modules would not work under mod_perl.
It took me a while but I don't think "our" works.  To test it, I inserted a couple fake code blocks in the new() function along with a global "my $count".  What I expected to see is the $count increment whenever a process got reused, but the unused code blocks in said process would be ignored.  Instead, the unused code blocks got displayed in a separate ticket.  It didn't matter if I used "our code_blocks;" or "our code_blocks = ();"

But when I used the original patch based on instance variables, it works as expected.
Hi Dylan, agreed.  Didn't read your comments when I submitted mine.  What about the other Markdown variables, g_nested_brackets, g_escape_table, etc?  They don't change per se.
(In reply to Dylan William Hardison [:dylan] from comment #12)
> What is true, is that you cannot use a file-scoped my inside a .cgi script.
> Inside a .pm is fine. If it wasn't, very many cpan modules would not work
> under mod_perl.


What I meant is that you cannot write:

my $foo;

sub bar {
   $foo++; # or anything else with $foo
}

because this triggers "Variable "$foo" will not stay shared" warnings, per http://perldoc.perl.org/perldiag.html#Variable-%22%s%22-will-not-stay-shared. That's what I was talking about.
Comment on attachment 8674708 [details] [diff] [review]
v2

So it seems this patch doesn't fix your problem per comment 13.
Attachment #8674708 - Attachment is obsolete: true
Attachment #8674708 - Flags: review?(LpSolit)
Attached patch v3 (obsolete) — Splinter Review
Updated version that doesn't use my/our.
Attachment #8675120 - Flags: review?(LpSolit)
Comment on attachment 8675120 [details] [diff] [review]
v3

>+sub indented_code_blocks {

This method is unused.
Attachment #8675120 - Flags: review?(LpSolit)
Attached patch v4 (obsolete) — Splinter Review
Removed indented_code_blocks, straggler from ticket 1205072
Attachment #8675120 - Attachment is obsolete: true
Attachment #8675155 - Flags: review?(LpSolit)
(In reply to Frédéric Buclin from comment #15)
> (In reply to Dylan William Hardison [:dylan] from comment #12)
> > What is true, is that you cannot use a file-scoped my inside a .cgi script.
> > Inside a .pm is fine. If it wasn't, very many cpan modules would not work
> > under mod_perl.
> 
> 
> What I meant is that you cannot write:
> 
> my $foo;
> 
> sub bar {
>    $foo++; # or anything else with $foo
> }
> 
> because this triggers "Variable "$foo" will not stay shared" warnings, per
> http://perldoc.perl.org/perldiag.html#Variable-%22%s%22-will-not-stay-shared.
> That's what I was talking about.

Only inside perl scripts loaded into the apache registry. That is not the case for random perl modules.
This is entirely because mod_perl (really, the registry handler) wraps scripts in "sub FOO { $script_content }".
This is not the case for any code in a .pm file.
Comment on attachment 8675155 [details] [diff] [review]
v4

>+sub code_blocks {

Please set it as a private method _code_blocks.


>+    return undef if $self->{'error'};

$self->{'error'} is never set anywhere and so doesn't exist. This line should go away.


>+    my @blocks = ();
>+    $self->{'code_blocks'} ||= \@blocks;
>+    # force param to hold the same reference, as Markdown rebuilds the object
>+    $self->{'params'}->{'code_blocks'} ||= \@blocks;

Merge all 3 lines into a single line:
  $self->{code_blocks} = $self->{params}->{code_blocks} ||= [];


Otherwise, this looks good but I didn't test it. Please ask koosha/dkl to review the updated patch as I don't use Markdown myself, and I wasn't involved in the original implementation of Markdown.
Attachment #8675155 - Flags: review?(LpSolit) → review-
Attached patch v5 (obsolete) — Splinter Review
Updated patch with the suggested fixes
Attachment #8675155 - Attachment is obsolete: true
Attachment #8675189 - Flags: review?(koosha.khajeh)
Comment on attachment 8675189 [details] [diff] [review]
v5

>+sub _code_blocks {
>+    my ($self) = @_;
>+    $self->{code_blocks} = $self->{params}->{code_blocks} ||= [];
>+    return $self->{'code_blocks'};
>+}

What is '$self->{params}->{code_blocks}' for? Why don't you just write:

  $self->{code_blocks} ||= [];

You could probably declare this variable with 'state' but I don't know how compatible that would be with mod_perl:

  state @{$self->{code_blocks}};

I do not know how to reproduce this bug.
(In reply to Koosha KM from comment #23)
> Comment on attachment 8675189 [details] [diff] [review]
> 
>   state @{$self->{code_blocks}};

I mean something like:

+sub _code_blocks {
+    state @blocks;
+    return @blocks;
+}
(In reply to Koosha KM from comment #24)
> (In reply to Koosha KM from comment #23)
> > Comment on attachment 8675189 [details] [diff] [review]
> > 
> >   state @{$self->{code_blocks}};
> 
> I mean something like:
> 
> +sub _code_blocks {
> +    state @blocks;
> +    return @blocks;
> +}

Using state like that would have exactly the same problem a file-scoped lexical. You want a new set of code blocks for reach instance of the parser.
Comment on attachment 8675189 [details] [diff] [review]
v5

Review of attachment 8675189 [details] [diff] [review]:
-----------------------------------------------------------------

a couple of thoughts related to this...

1) use_metadata should be forced off when we construct a Bugzilla::Markdown.
2) Instead of %%%FENCED_BLOCK%%%, a private-use unicode codepoint would be a good idea. Also removing the self-same unicode char before we begin processing the markdown
would be a good idea.

::: Bugzilla/Markdown.pm~
@@ +77,5 @@
>  }
> +sub _code_blocks {
> +    my ($self) = @_;
> +    $self->{code_blocks} = $self->{params}->{code_blocks} ||= [];
> +    return $self->{'code_blocks'};

nit: code_blocks does not need to be quoted here.
Attachment #8675189 - Flags: review+
Dylan, with the +review, do you want me to address the items in comment #26 or as a separate ticket?

>>+    $self->{code_blocks} = $self->{params}->{code_blocks} ||= [];
>>
> What is '$self->{params}->{code_blocks}' for?

Koosha, the "params" copy is a requirement because Text::Markdown (and MultiMarkdown) rebuilds the instance variables in new() and markdown() based on the params copy.  Perhaps it's a bug in Text::Markdown, but at this time, both have to be defined.
(In reply to Albert Ting from comment #27)
> Dylan, with the +review, do you want me to address the items in comment #26
> or as a separate ticket?

I'll file a bug about it. It isn't related to this bug -- and I don't think it has a security implication.
Thanks Dylan.  If it's ok, I already have an updated patch to bug 1205072 that uses unicode codepoint.
Attachment #8675189 - Flags: review?(koosha.khajeh)
The patch doesn't apply cleanly to master:

patch -p0 --dry-run < bug-1214310-code-block-leak
checking file Bugzilla/Markdown.pm
Hunk #3 FAILED at 75.
Hunk #4 succeeded at 119 (offset -49 lines).
Hunk #5 succeeded at 407 (offset -48 lines).
1 out of 5 hunks FAILED

Could you update your patch, please?
Assignee: ui → altlist
Status: NEW → ASSIGNED
Attached patch v6Splinter Review
Updated to work with master.  Also removed the unnecessary quote in the _code_blocks return statement
Attachment #8675189 - Attachment is obsolete: true
Attachment #8675768 - Flags: review?(dylan)
Attachment #8675768 - Flags: review?(LpSolit)
Attachment #8675768 - Flags: review?(LpSolit)
Comment on attachment 8675768 [details] [diff] [review]
v6

Review of attachment 8675768 [details] [diff] [review]:
-----------------------------------------------------------------

Checking the lifetime of the Markdown object, it's per-request. This means the $self->_code_blocks should be reset
for each request now (which is code). However, it might be possible to leak info from a hidden comment to a non-hidden comment?
Is it feasible to clear the code_blocks() inbetween calls to the markdown parser?

Tested against a fully-updated checkout. :-)

r=dylan
Attachment #8675768 - Flags: review?(dylan) → review+
(In reply to Dylan William Hardison [:dylan] from comment #32)
> However, it might be possible to leak
> info from a hidden comment to a non-hidden comment?

Is that possible (with STR) or just a guess? If it's reproducible, then this is still a security bug which must be fixed.
Dylan/Frederic,

The only way a code block could be used in a different comment is if there's a bug in the DoCodeBlocks code that somehow skips fetching a fenced code block.  That could happen if comment #26, bullet 2 was abused.  I was planning to address private unicode support as part of ticket 1205072.

Or as a failsafe, we could add this line of code in markdown(), to force a reset before each markdown call:

    # reset code blocks
    splice(@{$self->_code_blocks});
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   197090f..2814405  master -> master
Group: bugzilla-security
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1205072
For the record, the proposed v1 patch in bug 1154457 did cause a fenced block to be skipped if it was placed within an outline list.  

Given markdown has a number of open issues, it may be worth resetting the code_block array via the solution I noted in comment #34.  If agreeable, I'll create a separate patch.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: