CVE-2021-3852: Authorization Bypass Through User-Controlled Key (CWE-639)
value: growi is vulnerable to Authorization Bypass Through User-Controlled Key
CWE description: The system's authorization functionality does not prevent one user from gaining access to another user's data or record by modifying the key value identifying the data.
CVSS v3 base score: 7.5
Published: 2022-01-12T11:15Z
Repository: https://github.com/weseek/growi
Language: JavaScript
Files changed: packages/app/src/client/services/CommentContainer.js, packages/app/src/components/PageComment/Comment.jsx, packages/app/src/server/models/comment.js, packages/app/src/server/routes/comment.js
Commit date: 2021-10-08 20:19:17 +0900
Commit message: imprv: Permissions to operate comment (#4466)

* check author when updating and removing

* restrict to show controls when the user is not the author
Diff: diff --git a/packages/app/src/client/services/CommentContainer.js b/packages/app/src/client/services/CommentContainer.js
index a2b7fbfa6..6b7982703 100644
--- a/packages/app/src/client/services/CommentContainer.js
+++ b/packages/app/src/client/services/CommentContainer.js
@@ -130,15 +130,13 @@ export default class CommentContainer extends Container {
     const { pageId, revisionId } = this.getPageContainer().state;
 
     return this.appContainer.apiPost('/comments.update', {
       commentForm: {
         comment,
-        page_id: pageId,
-        revision_id: revisionId,
         is_markdown: isMarkdown,
+        revision_id: revisionId,
         comment_id: commentId,
-        author,
       },
     })
       .then((res) => {
         if (res.ok) {
           return this.retrieveComments();
diff --git a/packages/app/src/components/PageComment/Comment.jsx b/packages/app/src/components/PageComment/Comment.jsx
index c56fbce26..4015eae90 100644
--- a/packages/app/src/components/PageComment/Comment.jsx
+++ b/packages/app/src/components/PageComment/Comment.jsx
@@ -71,14 +71,10 @@ class Comment extends React.PureComponent {
     const { interceptorManager } = this.props.appContainer;
 
     interceptorManager.process('postRenderCommentHtml', this.currentRenderingContext);
   }
 
-  checkPermissionToControlComment() {
-    return this.props.appContainer.isAdmin || this.isCurrentUserEqualsToAuthor();
-  }
-
   isCurrentUserEqualsToAuthor() {
     const { creator } = this.props.comment;
     if (creator == null) {
       return false;
     }
@@ -208,11 +204,11 @@ class Comment extends React.PureComponent {
                   <UncontrolledTooltip placement="bottom" fade={false} target={`page-comment-revision-${commentId}`}>
                     {t('page_comment.display_the_page_when_posting_this_comment')}
                   </UncontrolledTooltip>
                 </span>
               </div>
-              {this.checkPermissionToControlComment() && (
+              {this.isCurrentUserEqualsToAuthor() && (
                 <CommentControl
                   onClickDeleteBtn={this.deleteBtnClickedHandler}
                   onClickEditBtn={() => this.setState({ isReEdit: true })}
                 />
               ) }
diff --git a/packages/app/src/server/models/comment.js b/packages/app/src/server/models/comment.js
index f413d9368..eddafe5cd 100644
--- a/packages/app/src/server/models/comment.js
+++ b/packages/app/src/server/models/comment.js
@@ -63,20 +63,10 @@ module.exports = function(crowi) {
         return resolve(data);
       });
     }));
   };
 
-  commentSchema.statics.updateCommentsByPageId = function(comment, isMarkdown, commentId) {
-    const Comment = this;
-
-    return Comment.findOneAndUpdate(
-      { _id: commentId },
-      { $set: { comment, isMarkdown } },
-    );
-
-  };
-
   commentSchema.statics.removeCommentsByPageId = function(pageId) {
     const Comment = this;
 
     return new Promise(((resolve, reject) => {
       Comment.remove({ page: pageId }, (err, done) => {
diff --git a/packages/app/src/server/routes/comment.js b/packages/app/src/server/routes/comment.js
index 5ec843c15..8ee13cffd 100644
--- a/packages/app/src/server/routes/comment.js
+++ b/packages/app/src/server/routes/comment.js
@@ -308,14 +308,14 @@ module.exports = function(crowi, app) {
    *                        properties:
    *                          page_id:
    *                            $ref: '#/components/schemas/Page/properties/_id'
    *                          revision_id:
    *                            $ref: '#/components/schemas/Revision/properties/_id'
+   *                          comment_id:
+   *                            $ref: '#/components/schemas/Comment/properties/_id'
    *                          comment:
    *                            $ref: '#/components/schemas/Comment/properties/comment'
-   *                          comment_position:
-   *                            $ref: '#/components/schemas/Comment/properties/commentPosition'
    *                required:
    *                  - form
    *        responses:
    *          200:
    *            description: Succeeded to update comment dody.
@@ -338,37 +338,45 @@ module.exports = function(crowi, app) {
    * @apiGroup Comment
    */
   api.update = async function(req, res) {
     const { commentForm } = req.body;
 
-    const pageId = commentForm.page_id;
-    const comment = commentForm.comment;
+    const commentStr = commentForm.comment;
     const isMarkdown = commentForm.is_markdown;
     const commentId = commentForm.comment_id;
-    const author = commentForm.author;
+    const revision = commentForm.revision_id;
 
-    if (comment === '') {
+    if (commentStr === '') {
       return res.json(ApiResponse.error('Comment text is required'));
     }
 
     if (commentId == null) {
       return res.json(ApiResponse.error('\'comment_id\' is undefined'));
     }
 
-    if (author !== req.user.username) {
-      return res.json(ApiResponse.error('Only the author can edit'));
-    }
-
-    // check whether accessible
-    const isAccessible = await Page.isAccessiblePageByViewer(pageId, req.user);
-    if (!isAccessible) {
-      return res.json(ApiResponse.error('Current user is not accessible to this page.'));
-    }
-
     let updatedComment;
     try {
-      updatedComment = await Comment.updateCommentsByPageId(comment, isMarkdown, commentId);
+      const comment = await Comment.findById(commentId).exec();
+
+      if (comment == null) {
+        throw new Error('This comment does not exist.');
+      }
+
+      // check whether accessible
+      const pageId = comment.page;
+      const isAccessible = await Page.isAccessiblePageByViewer(pageId, req.user);
+      if (!isAccessible) {
+        throw new Error('Current user is not accessible to this page.');
+      }
+      if (req.user.id !== comment.creator.toString()) {
+        throw new Error('Current user is not operatable to this comment.');
+      }
+
+      updatedComment = await Comment.findOneAndUpdate(
+        { _id: commentId },
+        { $set: { comment: commentStr, isMarkdown, revision } },
+      );
     }
     catch (err) {
       logger.error(err);
       return res.json(ApiResponse.error(err));
     }
@@ -436,10 +444,13 @@ module.exports = function(crowi, app) {
       const pageId = comment.page;
       const isAccessible = await Page.isAccessiblePageByViewer(pageId, req.user);
       if (!isAccessible) {
         throw new Error('Current user is not accessible to this page.');
       }
+      if (req.user.id !== comment.creator.toString()) {
+        throw new Error('Current user is not operatable to this comment.');
+      }
 
       await comment.removeWithReplies();
       await Page.updateCommentCount(comment.page);
     }
     catch (err) {

Vulnerable code:         page_id: pageId,
        revision_id: revisionId,
        author,
  checkPermissionToControlComment() {
    return this.props.appContainer.isAdmin || this.isCurrentUserEqualsToAuthor();
  }

              {this.checkPermissionToControlComment() && (
  commentSchema.statics.updateCommentsByPageId = function(comment, isMarkdown, commentId) {
    const Comment = this;

    return Comment.findOneAndUpdate(
      { _id: commentId },
      { $set: { comment, isMarkdown } },
    );

  };

   *                          comment_position:
   *                            $ref: '#/components/schemas/Comment/properties/commentPosition'
    const pageId = commentForm.page_id;
    const comment = commentForm.comment;
    const author = commentForm.author;
    if (comment === '') {
    if (author !== req.user.username) {
      return res.json(ApiResponse.error('Only the author can edit'));
    }

    // check whether accessible
    const isAccessible = await Page.isAccessiblePageByViewer(pageId, req.user);
    if (!isAccessible) {
      return res.json(ApiResponse.error('Current user is not accessible to this page.'));
    }

      updatedComment = await Comment.updateCommentsByPageId(comment, isMarkdown, commentId);
Fixed code:         revision_id: revisionId,
              {this.isCurrentUserEqualsToAuthor() && (
   *                          comment_id:
   *                            $ref: '#/components/schemas/Comment/properties/_id'
    const commentStr = commentForm.comment;
    const revision = commentForm.revision_id;
    if (commentStr === '') {
      const comment = await Comment.findById(commentId).exec();

      if (comment == null) {
        throw new Error('This comment does not exist.');
      }

      // check whether accessible
      const pageId = comment.page;
      const isAccessible = await Page.isAccessiblePageByViewer(pageId, req.user);
      if (!isAccessible) {
        throw new Error('Current user is not accessible to this page.');
      }
      if (req.user.id !== comment.creator.toString()) {
        throw new Error('Current user is not operatable to this comment.');
      }

      updatedComment = await Comment.findOneAndUpdate(
        { _id: commentId },
        { $set: { comment: commentStr, isMarkdown, revision } },
      );
      if (req.user.id !== comment.creator.toString()) {
        throw new Error('Current user is not operatable to this comment.');
      }