Reland "Make potentially dangerous Actions require a user click." This is a reland of 9d784c291714b703b16185e69860a3797de85b6c https://chromium-review.googlesource.com/c/chromium/src/+/1244367 was submitted changing the test that broke with this CL to not depend on PDF OpenActions anymore. Original change's description: > Make potentially dangerous Actions require a user click. > > URI and SubmitForm actions are only handled if the event was > ButtonUp or ButtonDown. > > Bug: 851821 > Change-Id: If6eb0ff44f6d62ac6df50b552c0bdc582885ab5d > Reviewed-on: https://pdfium-review.googlesource.com/42731 > Commit-Queue: Henrique Nakashima <hnakashima@chromium.org> > Reviewed-by: Tom Sepez <tsepez@chromium.org> > Reviewed-by: Ryan Harrison <rharrison@chromium.org> Bug: 851821 Change-Id: Iaf9c399059590f0f1a050ac450e08ee60a8d5a38 Reviewed-on: https://pdfium-review.googlesource.com/43410 Commit-Queue: Henrique Nakashima <hnakashima@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Ryan Harrison <rharrison@chromium.org>
diff --git a/core/fpdfdoc/cpdf_aaction.cpp b/core/fpdfdoc/cpdf_aaction.cpp index 9a95340..8d95469 100644 --- a/core/fpdfdoc/cpdf_aaction.cpp +++ b/core/fpdfdoc/cpdf_aaction.cpp
@@ -32,8 +32,9 @@ "DP", // DocumentPrinted }; -// |g_sAATypes| should have as many elements as enum AActionType. -static_assert(FX_ArraySize(g_sAATypes) == CPDF_AAction::NumberOfActions, +// |g_sAATypes| should have one less element than enum AActionType due to +// DocumentOpen, which is an artificial type. +static_assert(FX_ArraySize(g_sAATypes) == CPDF_AAction::NumberOfActions - 1, "g_sAATypes count mismatch"); } // namespace @@ -52,3 +53,14 @@ return CPDF_Action(m_pDict ? m_pDict->GetDictFor(g_sAATypes[eType]) : nullptr); } + +// static +bool CPDF_AAction::IsUserClick(AActionType eType) { + switch (eType) { + case ButtonUp: + case ButtonDown: + return true; + default: + return false; + } +}
diff --git a/core/fpdfdoc/cpdf_aaction.h b/core/fpdfdoc/cpdf_aaction.h index 11eca01..90049f5 100644 --- a/core/fpdfdoc/cpdf_aaction.h +++ b/core/fpdfdoc/cpdf_aaction.h
@@ -35,6 +35,7 @@ DocumentSaved, PrintDocument, DocumentPrinted, + DocumentOpen, NumberOfActions // Must be last. }; @@ -46,6 +47,8 @@ CPDF_Action GetAction(AActionType eType) const; const CPDF_Dictionary* GetDict() const { return m_pDict.Get(); } + static bool IsUserClick(AActionType eType); + private: UnownedPtr<const CPDF_Dictionary> const m_pDict; };
diff --git a/fpdfsdk/cpdfsdk_actionhandler.cpp b/fpdfsdk/cpdfsdk_actionhandler.cpp index f951bfb..f98585f 100644 --- a/fpdfsdk/cpdfsdk_actionhandler.cpp +++ b/fpdfsdk/cpdfsdk_actionhandler.cpp
@@ -82,7 +82,7 @@ CPDF_AAction::AActionType type, CPDFSDK_FormFillEnvironment* pFormFillEnv) { std::set<const CPDF_Dictionary*> visited; - return ExecuteBookMark(action, pFormFillEnv, pBookMark, &visited); + return ExecuteBookMark(action, type, pFormFillEnv, pBookMark, &visited); } bool CPDFSDK_ActionHandler::DoAction_Screen( @@ -96,9 +96,10 @@ bool CPDFSDK_ActionHandler::DoAction_Link( const CPDF_Action& action, + CPDF_AAction::AActionType type, CPDFSDK_FormFillEnvironment* pFormFillEnv) { std::set<const CPDF_Dictionary*> visited; - return ExecuteLinkAction(action, pFormFillEnv, &visited); + return ExecuteLinkAction(action, type, pFormFillEnv, &visited); } bool CPDFSDK_ActionHandler::DoAction_Field( @@ -130,7 +131,8 @@ RunDocumentOpenJavaScript(pFormFillEnv, L"", swJS); } } else { - DoAction_NoJs(action, pFormFillEnv); + DoAction_NoJs(action, CPDF_AAction::AActionType::DocumentOpen, + pFormFillEnv); } for (int32_t i = 0, sz = action.GetSubActionsCount(); i < sz; i++) { @@ -144,6 +146,7 @@ bool CPDFSDK_ActionHandler::ExecuteLinkAction( const CPDF_Action& action, + CPDF_AAction::AActionType eType, CPDFSDK_FormFillEnvironment* pFormFillEnv, std::set<const CPDF_Dictionary*>* visited) { const CPDF_Dictionary* pDict = action.GetDict(); @@ -159,12 +162,12 @@ context->OnLink_MouseUp(pFormFillEnv); }); } else { - DoAction_NoJs(action, pFormFillEnv); + DoAction_NoJs(action, eType, pFormFillEnv); } for (int32_t i = 0, sz = action.GetSubActionsCount(); i < sz; i++) { CPDF_Action subaction = action.GetSubAction(i); - if (!ExecuteLinkAction(subaction, pFormFillEnv, visited)) + if (!ExecuteLinkAction(subaction, eType, pFormFillEnv, visited)) return false; } @@ -190,7 +193,7 @@ RunDocumentPageJavaScript(pFormFillEnv, type, swJS); } } else { - DoAction_NoJs(action, pFormFillEnv); + DoAction_NoJs(action, type, pFormFillEnv); } ASSERT(pFormFillEnv); @@ -238,7 +241,7 @@ } } } else { - DoAction_NoJs(action, pFormFillEnv); + DoAction_NoJs(action, type, pFormFillEnv); } for (int32_t i = 0, sz = action.GetSubActionsCount(); i < sz; i++) { @@ -267,7 +270,7 @@ if (action.GetType() == CPDF_Action::JavaScript) RunScriptForAction(action, pFormFillEnv, [](IJS_EventContext*) {}); else - DoAction_NoJs(action, pFormFillEnv); + DoAction_NoJs(action, type, pFormFillEnv); for (int32_t i = 0, sz = action.GetSubActionsCount(); i < sz; i++) { CPDF_Action subaction = action.GetSubAction(i); @@ -280,6 +283,7 @@ bool CPDFSDK_ActionHandler::ExecuteBookMark( const CPDF_Action& action, + CPDF_AAction::AActionType type, CPDFSDK_FormFillEnvironment* pFormFillEnv, CPDF_Bookmark* pBookmark, std::set<const CPDF_Dictionary*>* visited) { @@ -296,12 +300,12 @@ context->OnBookmark_MouseUp(pBookmark); }); } else { - DoAction_NoJs(action, pFormFillEnv); + DoAction_NoJs(action, type, pFormFillEnv); } for (int32_t i = 0, sz = action.GetSubActionsCount(); i < sz; i++) { CPDF_Action subaction = action.GetSubAction(i); - if (!ExecuteBookMark(subaction, pFormFillEnv, pBookmark, visited)) + if (!ExecuteBookMark(subaction, type, pFormFillEnv, pBookmark, visited)) return false; } @@ -310,6 +314,7 @@ void CPDFSDK_ActionHandler::DoAction_NoJs( const CPDF_Action& action, + CPDF_AAction::AActionType type, CPDFSDK_FormFillEnvironment* pFormFillEnv) { ASSERT(pFormFillEnv); @@ -318,7 +323,8 @@ DoAction_GoTo(pFormFillEnv, action); break; case CPDF_Action::URI: - DoAction_URI(pFormFillEnv, action); + if (CPDF_AAction::IsUserClick(type)) + DoAction_URI(pFormFillEnv, action); break; case CPDF_Action::Hide: DoAction_Hide(action, pFormFillEnv); @@ -327,7 +333,8 @@ DoAction_Named(pFormFillEnv, action); break; case CPDF_Action::SubmitForm: - DoAction_SubmitForm(action, pFormFillEnv); + if (CPDF_AAction::IsUserClick(type)) + DoAction_SubmitForm(action, pFormFillEnv); break; case CPDF_Action::ResetForm: DoAction_ResetForm(action, pFormFillEnv);
diff --git a/fpdfsdk/cpdfsdk_actionhandler.h b/fpdfsdk/cpdfsdk_actionhandler.h index 14cbc9c..94b347f 100644 --- a/fpdfsdk/cpdfsdk_actionhandler.h +++ b/fpdfsdk/cpdfsdk_actionhandler.h
@@ -44,6 +44,7 @@ CPDFSDK_FormFillEnvironment* pFormFillEnv, CPDFSDK_Annot* pScreen); bool DoAction_Link(const CPDF_Action& action, + CPDF_AAction::AActionType type, CPDFSDK_FormFillEnvironment* pFormFillEnv); bool DoAction_Field(const CPDF_Action& action, CPDF_AAction::AActionType type, @@ -85,14 +86,17 @@ CPDFSDK_Annot* pScreen, std::set<const CPDF_Dictionary*>* visited); bool ExecuteBookMark(const CPDF_Action& action, + CPDF_AAction::AActionType type, CPDFSDK_FormFillEnvironment* pFormFillEnv, CPDF_Bookmark* pBookmark, std::set<const CPDF_Dictionary*>* visited); bool ExecuteLinkAction(const CPDF_Action& action, + CPDF_AAction::AActionType type, CPDFSDK_FormFillEnvironment* pFormFillEnv, std::set<const CPDF_Dictionary*>* visited); void DoAction_NoJs(const CPDF_Action& action, + CPDF_AAction::AActionType type, CPDFSDK_FormFillEnvironment* pFormFillEnv); void RunDocumentPageJavaScript(CPDFSDK_FormFillEnvironment* pFormFillEnv, CPDF_AAction::AActionType type,
diff --git a/fpdfsdk/cpdfsdk_widget.cpp b/fpdfsdk/cpdfsdk_widget.cpp index 8bd1f10..82776e3 100644 --- a/fpdfsdk/cpdfsdk_widget.cpp +++ b/fpdfsdk/cpdfsdk_widget.cpp
@@ -174,6 +174,7 @@ case CPDF_AAction::PrintDocument: case CPDF_AAction::DocumentPrinted: break; + case CPDF_AAction::DocumentOpen: case CPDF_AAction::NumberOfActions: NOTREACHED(); break;
diff --git a/fpdfsdk/fpdf_formfill_embeddertest.cpp b/fpdfsdk/fpdf_formfill_embeddertest.cpp index 8ff3a84..3e53753 100644 --- a/fpdfsdk/fpdf_formfill_embeddertest.cpp +++ b/fpdfsdk/fpdf_formfill_embeddertest.cpp
@@ -379,6 +379,25 @@ UnloadPage(page); } +class DoURIActionBlockedDelegate final : public EmbedderTest::Delegate { + public: + void DoURIAction(FPDF_BYTESTRING uri) override { + FAIL() << "Navigated to " << uri; + } +}; + +TEST_F(FPDFFormFillEmbeddertest, BUG_851821) { + DoURIActionBlockedDelegate delegate; + SetDelegate(&delegate); + + EXPECT_TRUE(OpenDocument("redirect.pdf")); + FPDF_PAGE page = LoadPage(0); + EXPECT_TRUE(page); + DoOpenActions(); + + UnloadPage(page); +} + #ifdef PDF_ENABLE_V8 TEST_F(FPDFFormFillEmbeddertest, DisableJavaScript) { // Test that timers and intervals can't fire without JS.
diff --git a/testing/embedder_test.cpp b/testing/embedder_test.cpp index 215009c..43f077c 100644 --- a/testing/embedder_test.cpp +++ b/testing/embedder_test.cpp
@@ -241,6 +241,8 @@ formfillinfo->FFI_SetTimer = SetTimerTrampoline; formfillinfo->FFI_KillTimer = KillTimerTrampoline; formfillinfo->FFI_GetPage = GetPageTrampoline; + formfillinfo->FFI_DoURIAction = DoURIActionTrampoline; + if (javascript_option == JavaScriptOption::kEnableJavaScript) formfillinfo->m_pJsPlatform = platform; @@ -478,6 +480,13 @@ } // static +void EmbedderTest::DoURIActionTrampoline(FPDF_FORMFILLINFO* info, + FPDF_BYTESTRING uri) { + EmbedderTest* test = static_cast<EmbedderTest*>(info); + return test->delegate_->DoURIAction(uri); +} + +// static std::string EmbedderTest::HashBitmap(FPDF_BITMAP bitmap) { uint8_t digest[16]; CRYPT_MD5Generate(static_cast<uint8_t*>(FPDFBitmap_GetBuffer(bitmap)),
diff --git a/testing/embedder_test.h b/testing/embedder_test.h index 6470763..a6a4cf5 100644 --- a/testing/embedder_test.h +++ b/testing/embedder_test.h
@@ -62,6 +62,9 @@ virtual FPDF_PAGE GetPage(FPDF_FORMFILLINFO* info, FPDF_DOCUMENT document, int page_index); + + // Equivalent to FPDF_FORMFILLINFO::FFI_DoURIAction(). + virtual void DoURIAction(FPDF_BYTESTRING uri) {} }; EmbedderTest(); @@ -245,6 +248,8 @@ static FPDF_PAGE GetPageTrampoline(FPDF_FORMFILLINFO* info, FPDF_DOCUMENT document, int page_index); + static void DoURIActionTrampoline(FPDF_FORMFILLINFO* info, + FPDF_BYTESTRING uri); static int WriteBlockCallback(FPDF_FILEWRITE* pFileWrite, const void* data, unsigned long size);
diff --git a/testing/resources/redirect.pdf b/testing/resources/redirect.pdf new file mode 100644 index 0000000..517bd5a --- /dev/null +++ b/testing/resources/redirect.pdf
@@ -0,0 +1,22 @@ +%PDF-1.7 +trailer +<< +/Root 1 0 R +>> +1 0 obj +<< +/Type /Catalog +/Pages 2 0 R +/OpenAction 2 0 R +>> +endobj + +2 0 obj +<< +/Type /Action +/S /URI +/URI (http://evilzone.org) // URL HERE +>> +endobj + +%%EOF