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)
Tracking
()
RESOLVED
FIXED
Bugzilla 6.0
People
(Reporter: altlist, Assigned: altlist)
References
Details
(Keywords: regression)
Attachments
(1 file, 5 obsolete files)
1.37 KB,
patch
|
dylan
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
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
Updated•9 years ago
|
Summary: Markdown code blocks can display comments from other tickets → Markdown code blocks can display comments from other bugs
Target Milestone: --- → Bugzilla 6.0
Assignee | ||
Comment 3•9 years ago
|
||
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?
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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.
Comment 6•9 years ago
|
||
(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
Assignee | ||
Comment 7•9 years ago
|
||
Is it sufficient to just use "our" instead of "my"? Or should the array be treated as an instance variable?
Comment 8•9 years ago
|
||
(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().
Assignee | ||
Comment 9•9 years ago
|
||
Updated patch that uses "our"
Attachment #8674330 -
Attachment is obsolete: true
Attachment #8674330 -
Flags: review?(koosha.khajeh)
Attachment #8674708 -
Flags: review?(LpSolit)
Comment 10•9 years ago
|
||
To be sure, does this fix your problem?
Comment 11•9 years ago
|
||
(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.
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
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.
Comment 15•9 years ago
|
||
(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 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
Updated version that doesn't use my/our.
Attachment #8675120 -
Flags: review?(LpSolit)
Comment 18•9 years ago
|
||
Comment on attachment 8675120 [details] [diff] [review] v3 >+sub indented_code_blocks { This method is unused.
Attachment #8675120 -
Flags: review?(LpSolit)
Assignee | ||
Comment 19•9 years ago
|
||
Removed indented_code_blocks, straggler from ticket 1205072
Attachment #8675120 -
Attachment is obsolete: true
Attachment #8675155 -
Flags: review?(LpSolit)
Comment 20•9 years ago
|
||
(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 21•9 years ago
|
||
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-
Assignee | ||
Comment 22•9 years ago
|
||
Updated patch with the suggested fixes
Attachment #8675155 -
Attachment is obsolete: true
Attachment #8675189 -
Flags: review?(koosha.khajeh)
Comment 23•9 years ago
|
||
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.
Comment 24•9 years ago
|
||
(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; +}
Comment 25•9 years ago
|
||
(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 26•9 years ago
|
||
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+
Assignee | ||
Comment 27•9 years ago
|
||
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.
Comment 28•9 years ago
|
||
(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.
Assignee | ||
Comment 29•9 years ago
|
||
Thanks Dylan. If it's ok, I already have an updated patch to bug 1205072 that uses unicode codepoint.
Updated•9 years ago
|
Attachment #8675189 -
Flags: review?(koosha.khajeh)
Comment 30•9 years ago
|
||
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
Assignee | ||
Comment 31•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8675768 -
Flags: review?(LpSolit)
Comment 32•9 years ago
|
||
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+
Comment 33•9 years ago
|
||
(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.
Assignee | ||
Comment 34•9 years ago
|
||
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});
Comment 35•9 years ago
|
||
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
Assignee | ||
Comment 36•9 years ago
|
||
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.
Description
•